Messages in this thread | | | Date | Thu, 25 Sep 2014 21:27:23 +0200 | Subject | Re: [PATCHv2 2/3] x86, ptdump: Simplify page flag evaluation code | From | Mathias Krause <> |
| |
On 24 September 2014 09:45, Ingo Molnar <mingo@kernel.org> wrote: > * Mathias Krause <minipli@googlemail.com> wrote: >> On 21 September 2014 21:49, Arjan van de Ven <arjan@linux.intel.com> 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): > > Both can be used (although I too find the original version easier > to read), and it's usually the taste/opinion of the original > author whose choice we prefer.
So I should start writing more code from the scratch than changing others... ;)
But concerning this patch, are you interested in the following other pieces: - changing the macros from being a compound statement expression to the common 'do .. while(0)' pattern - use of pr_info()/pr_cont() instead of printk(KERN_INFO/KERN_CONT) - use of format strings in pt_dump_cont_printf() calls - removing the trailing blank before '\n' in the "... # entries skipped ..." message ...or should I just drop patch 2 altogether?
Thanks, Mathias
| |