Re: [Xen-devel] [PATCH] console: avoid printing no or null time stamps

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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, <cardoe@xxxxxxxxxx> 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.grall@xxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel