Date:   Sat, 1 Apr 2017 04:32:39 +0100
From:   Al Viro <viro@...IV.linux.org.uk>
To:     Chewie Lin <linsh@...gonstate.edu>
Cc:     greg@...ah.com, forest@...ttletooquiet.net,
        devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch
 warning

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".

Powered by blists - more mailing lists