On 03/07/18 11:47, Julien Grall wrote:
>
>
> On 03/07/18 07:29, Jan Beulich wrote:
>>>>> On 02.07.18 at 22:28, <car...@cardoe.com> wrote:
>>> On Mon, Jul 02, 2018 at 02:47:42PM +0100, Julien Grall wrote:
>>>> On 06/26/2018 10:03 AM, Jan Beulich wrote:
>>>>>>>> On 26.06.18 at 10:43, <julien.gr...@arm.com> wrote:
>>>>>> On 26/06/18 08:24, Jan Beulich wrote:
>>>>>>> @@ -698,26 +701,30 @@ static void printk_start_of_line(const c
>>>>>>> case TSM_DATE_MS:
>>>>>>> tm = wallclock_time(&nsec);
>>>>>>> - if ( tm.tm_mday == 0 )
>>>>>>> - return;
>>>>>>> -
>>>>>>> - if ( opt_con_timestamp_mode == TSM_DATE )
>>>>>>> - snprintf(tstr, sizeof(tstr), "[%04u-%02u-%02u
>>>>>>> %02u:%02u:%02u] ",
>>>>>>> - 1900 + tm.tm_year, tm.tm_mon + 1, tm.tm_mday,
>>>>>>> - tm.tm_hour, tm.tm_min, tm.tm_sec);
>>>>>>> - else
>>>>>>> + if ( tm.tm_mday )
>>>>>>> + {
>>>>>>> snprintf(tstr, sizeof(tstr),
>>>>>>> - "[%04u-%02u-%02u
>>>>>>> %02u:%02u:%02u.%03"PRIu64"] ",
>>>>>>> + opt_con_timestamp_mode == TSM_DATE
>>>>>>> + ? "[%04u-%02u-%02u %02u:%02u:%02u] "
>>>>>>> + : "[%04u-%02u-%02u
>>>>>>> %02u:%02u:%02u.%03"PRIu64"] ",
>>>>>>> 1900 + tm.tm_year, tm.tm_mon + 1,
>>>>>>> tm.tm_mday,
>>>>>>> tm.tm_hour, tm.tm_min, tm.tm_sec, nsec
>>>>>>> / 1000000);
>>>>>>
>>>>>> I find this change rather difficult to read because the number of
>>>>>> arguments for the 2 formats are different. It would be better to
>>>>>> keep
>>>>>> the two snprintf separately.
>>>>>
>>>>> And I find the redundancy rather ugly to maintain, so I'd prefer
>>>>> to stick to
>>>>> single invocation.
>>>>
>>>> Maybe it is for you. Not for me.
>>>
>>> I'm in agreement with Julien. Its not easy to follow and certainly not
>>> having the number of arguments line up looks like a "code smell" to me.
>>
>> Having looked again, do both of you realize that I didn't do this change
>> just because I like it better this way, but first and foremost to avoid
>> another level of indentation (plus to make more obvious the similarity
>> between the two format strings)? The alternative of
>>
>> if ( tm.tm_mday == 0 )
>> /* nothing */;
>> else if ( opt_con_timestamp_mode == TSM_DATE )
>> {
>> ...
>>
>> (to also avoid the extra level) doesn't look better to me.
>
> Yes, I realized that when writing the e-mail yesterday. The extra
> level would not looked so bad compare to potential format mismatch not
> detected by the compiler.
I also agree with Julien. Having a situation where the compiler can't
typecheck the format string is far worse than other style concerns.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
