Messages in this thread | | | Date | Fri, 26 Sep 2014 11:25:39 +0200 | From | Ingo Molnar <> | Subject | Re: [PATCHv2 2/3] x86, ptdump: Simplify page flag evaluation code |
| |
* Mathias Krause <minipli@googlemail.com> wrote:
> 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?
Honestly? I'm not interested at all in self-centered make-work cleanups that don't really improve the code materially - I'm more interested in cleanups that are a side effect of real work and real interest.
There's literally 1 million checkpatch 'failures' in the kernel source code, do we really want to clean them all up, waste reviewer and maintainer bandwidth and pretend that those 1 million extra cleanup commits are just as valuable as the 'other' work that goes on in the kernel?
Why don't you do something different: for example something functionally useful (some real improvements to the code!), and make sure it's all clean while you are doing it? That gives an opportunity to clean up anything that your changes touch as well.
We always welcome cleanups that are a side effect of feature work. Cleanups for code that is perfectly readable, just for cleanup's sake, is counterproductive ...
Doing printk() formatting fixes is fine for a newbie, but as I told Perches a couple of times already, and as I told Bunk before him, most kernel developers grow beyond printk() fixes after their first few commits.
Thanks,
Ingo
| |