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    [W:0.032 / U:60.224 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site