Re: [PATCHv2 2/3] x86, ptdump: Simplify page flag evaluation code

[Mathias Krause]

On 21 September 2014 21:49, Arjan van de Ven <arjan@xxxxxxxxxxxxxxx> wrote:
> On 9/21/2014 8:26 AM, Mathias Krause wrote:
>>
>> -               if (pr & _PAGE_PCD)
>> -                       pt_dump_cont_printf(m, dmsg, "PCD ");
>> -               else
>> -                       pt_dump_cont_printf(m, dmsg, "    ");
>> +               pt_dump_cont(m, dmsg, "%-4s", pr & _PAGE_USER ? "USR" :
>> "");
>
>
> while you have some nice cleanups in your patch, I can't say I consider this
> an improvement.
> Yes the C standard allows ? to be used like this
> but no, I don't think it improves readability in general.

Not in general, but in this case, it does, IMHO.

> (I think for me the main exception is NULL pointer cases, but this is not
> one of these)

Apparently such a pattern (using the question mark operator combined
with a bit test to choose string constants) is used quite often in the
linux kernel as a simple grep tells me (probably catches a few false
positives but still a representative number):

$ git grep '[^&]&[^&].*? *"' | wc -l
2668

And, honestly, the bit test combined with the question mark operator
makes the code way more readable for me. It not only makes the code
more compact (1 instead of 4 lines). It also allows to have the common
parts written only once and thereby removing the possibility of having
them conflict with each other, e.g. make them generate strings of
different lengths.

Would you prefer the bit test to be surrounded by braces to make it
more easy to understand? Even though, the operator precedence is
clearly defined by the C standard for this case, so no braces are
needed.


Thanks,
Mathias
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/