lkml.org 
[lkml]   [2011]   [Jul]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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@kernel.org> wrote:
> > > > > > > On Sun, Jul 10, 2011 at 7:24 PM, Arnaud Lacombe <lacombar@gmail.com> wrote:
> > > > > > >> Hi,
> > > > > > >>
> > > > > > >> On Sun, Jul 10, 2011 at 12:16 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
> > > > > > >>> On Sun, Jul 10, 2011 at 12:24 AM, Raghavendra D Prabhu
> > > > > > >>> <rprabhu@wnohang.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@wnohang.net>
> > > > > > >>>
> > > > > > >>> Acked-by: WANG Cong <xiyou.wangcong@gmail.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@chaosbits.net> http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.
\
 
 \ /
  Last update: 2011-07-11 22:35    [from the cache]
©2003-2011 Jasper Spaans