Re: [PATCH] Avoid Wunused-but-set warning

[Randy Dunlap]

On Mon, 11 Jul 2011 01:21:36 +0200 (CEST) Jesper Juhl wrote:

> On Sun, 10 Jul 2011, Randy Dunlap wrote:
> 
> > On Mon, 11 Jul 2011 00:56:57 +0200 (CEST) Jesper Juhl wrote:
> > 
> > > On Sun, 10 Jul 2011, Randy Dunlap wrote:
> > > 
> > > > On Sun, 10 Jul 2011 12:49:20 -0400 Arnaud Lacombe wrote:
> > > > 
> > > > > Hi,
> > > > > 
> > > > > On Sun, Jul 10, 2011 at 12:28 PM, Pekka Enberg <penberg@xxxxxxxxxx> wrote:
> > > > > > On Sun, Jul 10, 2011 at 7:24 PM, Arnaud Lacombe <lacombar@xxxxxxxxx> wrote:
> > > > > >> Hi,
> > > > > >>
> > > > > >> On Sun, Jul 10, 2011 at 12:16 PM, Américo Wang <xiyou.wangcong@xxxxxxxxx> wrote:
> > > > > >>> On Sun, Jul 10, 2011 at 12:24 AM, Raghavendra D Prabhu
> > > > > >>> <rprabhu@xxxxxxxxxxx> wrote:
> > > > > >>>> Hi,
> > > > > >>>>    I am seeing Wunused-but-set warning while make nconfig.  Looks like
> > > > > >>>>    active_menu is not used. Removing it fixes the warning.
> > > > > >>>>
> > > > > >>>> Signed-off-by: Raghavendra D Prabhu <rprabhu@xxxxxxxxxxx>
> > > > > >>>
> > > > > >>> Acked-by: WANG Cong <xiyou.wangcong@xxxxxxxxx>
> > > > > >>>
> > > > > >> Out of curiosity, what is your status to ACK such patch ?
> > > > > >
> > > > > > What kind of status do you need to ACK such a simple patch?
> > > > > >
> > > > > As per Documentation/SubmittingPatches:
> > > > > 
> > > > > <<
> > > > > 13) When to use Acked-by: and Cc:
> > > > > The Signed-off-by: tag indicates that the signer was involved in the
> > > > > development of the patch, or that he/she was in the patch's delivery path.
> > > > > 
> > > > > If a person was not directly involved in the preparation or handling of a
> > > > > patch but wishes to signify and record their approval of it then they can
> > > > > arrange to have an Acked-by: line added to the patch's changelog.
> > > > > 
> > > > > Acked-by: is often used by the maintainer of the affected code when that
> > > > > maintainer neither contributed to nor forwarded the patch.
> > > > > >>
> > > > > 
> > > > > That said, it is not a strong requirement... unfortunately. So, let's
> > > > > have some fun and go ACK thousand of trivial patch just to generate
> > > > > traffic on the LKML and give myself self-importance :-)
> > > > 
> > > > Acked-by: is mostly used as a weak version of Reviewed-by:
> > > > and the "definition" in SubmittingPatches is not accurate IMO.
> > > > I.e., it can be used by anyone.
> > > > 
> > > 
> > > Interesting. I was under the impression that Reviewed-by: was a weaker 
> > > thing than Acked-by: - I certainly have been using it as such.
> > > 
> > > I've always interpreted Acked-by: as being something you could apply if 
> > > you were the author, maintainer or other person with similar strong 
> > > background knowledge of the code. Where Reviewed-by: could be used by 
> > > anyone, as long as they had taken the time to read the patch and try and 
> > > understand what was going on and the result/conclusion looked good.
> > 
> > I don't see it in SubmittingPatches, but there was some discussion at the
> > time (IIRC!!) that Reviewed-by: indicates that you are willing to support/fix
> > the patch if the patch author(s) disappear.  I.e., you are willing to take
> > some ownership responsibility of the patch.
> > 
> > or I could be dreaming...
> > 
> I'm not going to claim that I recall all the discussion that went on at 
> the time, there was quite a bit IIRC (and I'm too lazy to read up on all 
> of it). But to me it seems to make sense that if you have strong knowledge 
> of/involvement with the code being patched then you can offer your 
> Acked-by: after reviewing the patch. If you don't have such 
> knowledge/involvement but have nevertheless reviewed the code and found it 
> to be OK, then you can signal that with a Reviewed-by:.
> 
> In any case, you can't expect people to base their Acked-by/Reviewed-by 
> replies on some conclusion in some email thread that happened years ago 

ack that.

> but was never written down in some document in the repository. 
> It is only reasonable to expect people to behave according to the rules 
> laid out in SubmittingPatches and similar documents, and those rules 
> currently seem to support my interpretation, not yours.

In Documentation/SubmittingPatches, Reviewed-by: contains a "Reviewer's
statment of oversight."  That alone is more formal than Acked-by: is.

Plus this paragraph acknowledges that "Review" can be a serious and
time-consuming task, not a simple "looks OK to me":

"A Reviewed-by tag is a statement of opinion that the patch is an
appropriate modification of the kernel without any remaining serious
technical issues.  Any interested reviewer (who has done the work) can
offer a Reviewed-by tag for a patch.  This tag serves to give credit to
reviewers and to inform maintainers of the degree of review which has been
done on the patch.  Reviewed-by: tags, when supplied by reviewers known to
understand the subject area and to perform thorough reviews, will normally
increase the likelihood of your patch getting into the kernel."

I can see little about Acked-by: that is formal when it comes to patch review.
E.g.:
'Acked-by: is not as formal as Signed-off-by:.  It is a record that the acker
has at least reviewed the patch and has indicated acceptance.  Hence patch
mergers will sometimes manually convert an acker's "yep, looks good to me"
into an Acked-by:.'

But do as you like.  Which parts of SubmittingPatches do you think
support your interpretation?

and should we have this line:
	Acked-by: is not as formal as Signed-off-by:.
changed to:
	Acked-by: is not as formal as Signed-off-by: or Reviewed-by:.
e.g.?

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
--
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/