Date:	Mon, 11 Jul 2011 22:32:58 +0200 (CEST)
From:	Jesper Juhl <jj@...osbits.net>
To:	Randy Dunlap <rdunlap@...otime.net>
cc:	Arnaud Lacombe <lacombar@...il.com>,
	Pekka Enberg <penberg@...nel.org>,
	Américo Wang <xiyou.wangcong@...il.com>,
	Raghavendra D Prabhu <rprabhu@...hang.net>,
	linux-kbuild@...r.kernel.org, Nir Tzachar <nir.tzachar@...il.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Avoid Wunused-but-set warning

On Sun, 10 Jul 2011, Randy Dunlap wrote:

> 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@...nel.org> wrote:
> > > > > > > On Sun, Jul 10, 2011 at 7:24 PM, Arnaud Lacombe <lacombar@...il.com> wrote:
> > > > > > >> Hi,
> > > > > > >>
> > > > > > >> On Sun, Jul 10, 2011 at 12:16 PM, Américo Wang <xiyou.wangcong@...il.com> wrote:
> > > > > > >>> On Sun, Jul 10, 2011 at 12:24 AM, Raghavendra D Prabhu
> > > > > > >>> <rprabhu@...hang.net> 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@...hang.net>
> > > > > > >>>
> > > > > > >>> Acked-by: WANG Cong <xiyou.wangcong@...il.com>
> > > > > > >>>
> > > > > > >> 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:.'
> 

I'll concede to those points. My original understanding/reading was 
different but you make some good points.
 

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

Originally, text such as this:

"Acked-by: is often used by the maintainer of the affected code..."

"Reviewed-by:, instead, indicates that the patch has been reviewed and 
found acceptable ... I do not (unless explicitly stated elsewhere) make 
any warranties or guarantees that it will achieve its stated purpose ..."

But I take that back. Your interpretation now seems like the more correct 
one to me.

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

Might make sense.

-- 
Jesper Juhl <jj@...osbits.net>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

Powered by blists - more mailing lists