On Sat, Apr 01, 2017 at 04:32:39AM +0100, Al Viro wrote:
> On Fri, Mar 31, 2017 at 06:59:19PM -0700, Chewie Lin wrote:
> > Replace string with formatted arguments in the dev_warn() call. It removes
> > the checkpatch warning:
> >
> > WARNING: Prefer using "%s", __func__ to embedded function names
> > #417: FILE: main_usb.c:417:
> > + "usb_device_reset fail status=%d\n", status);
> >
> > total: 0 errors, 1 warnings, 1058 lines checked
> >
> > And after fix:
> >
> > main_usb.c has no obvious style problems and is ready for submission.
>
>
> > - "usb_device_reset fail status=%d\n", status);
> > + "%s=%d\n", "usb_device_reset fail status", status);
>
> Would you mind explaining the meaning of that warning? Incidentally, why not
> "%se_reset fail status=%d\n", "usb_devic", status);
> - it would produce the identical output and silences checkpatch even more
> reliably. Discuss.
>
> Sarcasm aside, when you are proposing a change, there are several questions you
> really must ask yourself and be able to answer. First and foremost, of course,
> is
> * Why is it an improvement?
> If the answer to that was "it makes a warning go away", the next questions are
> * What are we warned about?
> * What is problematic in the original variant?
> * Is the replacement free from the problem we had in the original?
> and if the answer to any of that is "I don't know", the next step is _not_
> "send it anyway". "Try to figure out" might be a good idea, with usual
> heuristics regarding the reading comprehension ("if a sentence remains
> a mystery, see if there are any unfamiliar words skipped at the first reading
> and find what they mean", etc.) applying.
>
> In this particular case the part you've apparently skipped was, indeed,
> critical - "__func__". I am not trying to defend the quality of checkpatch -
> the warning is badly worded, the tool is badly written and more often than
> not its suggestions reflect nothing but authors' arbitrary preferences.
>
> This one, however, does have some rationale behind it. Namely, if some
> debugging output contains the name of a function that has produced it,
> having that name spelled out in format string is not a good idea. If
> that code gets moved elsewhere one would have to replace the name in format
> string or face confusing messages refering to the function where that
> code used to be. Since __func__ is interpreted as a constant string
> initialized with the name of the function it's used in, it is possible
> to avoid spelling the function name out - instead of
> "my_function: pink elephants; reduce the daily intake to %d glasses\n", n
> one could use
> "%s: pink elephants; reduce the daily intake to %d glasses\n", __func__, n
> producing the same output and avoiding the need to adjust anything when
> the whole thing is moved to another function. It's not particularly big
> deal, but it's a more or less reasonable suggestion.
>
> Your change, of course, has achieved only one thing - it has moved the
> explicitly spelled out function name out of format string. It's still
> just as explicitly spelled out, still would need to be adjusted if moved
> to another function and the whole thing has become harder to read and
> understand since you've buried other parts of message in the place where
> it's harder to follow.
>
> Again, checkpatch warning is badly written, but the main problem with
> your posting is not that you had been confused by checkpatch - it's
> that you have posted it based on an incomprehensible message with no better
> rationale than "it shuts checkpatch up, dunno why and what about".
Al, brilliant, that's exactly what I was trying to do on my first try.
The checkpatch *is* confusing. It's fine with a simple string but doesn't
like it when it's formatted string. From what you said, I
think this may work better and more portable:
"%s: fail status = %d\n", "usb_device_reset", status)
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
