Re: [PATCH] trace.c: mark file-local function static

From: Drew Northup <drew.northup@maine.edu>
Date: Dec 21, 2010, 12:29 UTC
Message-ID: <1292934581.26698.14.camel@drew-northup.unet.maine.edu>
In-Reply-To: <AANLkTin2QuMF93RqrRcScxvkzhU4OFJ9Nt42tV+FOjxS@mail.gmail.com>

On Mon, 2010-12-20 at 22:28 -0200, Thiago Farina wrote:

Show 44 quoted lines

> On Mon, Dec 20, 2010 at 2:53 PM, Drew Northup <drew.northup@maine.edu> wrote:
> >
> > On Mon, 2010-12-20 at 13:17 -0200, Thiago Farina wrote:
> >> On Mon, Dec 20, 2010 at 10:00 AM, Drew Northup <drew.northup@maine.edu> wrote:
> >> >
> >> > On Thu, 2010-12-16 at 21:43 -0200, Thiago Farina wrote:
> >> >> On Thu, Dec 16, 2010 at 8:38 PM, Vasyl' <vvavrychuk@gmail.com> wrote:
> >> >> > Signed-off-by: Vasyl' Vavrychuk <vvavrychuk@gmail.com>
> >> >> > ---
> >> >> >  trace.c |    2 +-
> >> >> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >> >> >
> >> >> > diff --git a/trace.c b/trace.c
> >> >> > index 1e560cb..62586fa 100644
> >> >> > --- a/trace.c
> >> >> > +++ b/trace.c
> >> >> > @@ -25,7 +25,7 @@
> >> >> >  #include "cache.h"
> >> >> >  #include "quote.h"
> >> >> >
> >> >> > -void do_nothing(size_t unused)
> >> >> > +static void do_nothing(size_t unused)
> >> >> >  {
> >> >> >  }
> >> >> >
> >> >> If it means something, this looks sane to me.
> >> >>
> >> >> Acked-by: Thiago Farina <tfransosi@gmail.com>
> >> >
> >> > It may be sane, but why should we trust that it is without a commit
> >> > message?
> >>
> >> Why such trivial thing needs further explanation?
> >
> > Because even trivial fixes may break non-trivial things.
> > In addition, without justification we'd just as soon have somebody come
> > back with another patch six months down the road that changes it back to
> > the original code. Now that wouldn't make a whole lot of sense, now
> > would it?
> 
> I don't think so, it's making the function private, because the
> function is used only in that file and as such if you see a function
> marked as static you know that and doesn't need further explanation in
> my pov (but it seems you don't think like that).

All the patch above does is tell the compiler to enforce compilation of that function as static. That is most definitely not the same thing as making it private. (I think we can agree that it is being used as if it were being enforced as private at some meta-level, but the compiler isn't going to enforce it for us...) While I think the code change is fairly clear, as I said earlier: without a commit message we don't have a good reason for not making it non-static again later on, flip-flopping ad-infinitum.

Commit messages for isolated changes such as this build up a story, if you will, providing future contributors with insight as to why the group made a change when it did--even when the change is minor (in fact often most importantly when the change is minor)--by putting it in context.

Show 7 quoted lines

> > Alas the best way to avoid such a situation is to explain why a change
> > was made to begin with.
> >
> 
> So, you are welcome to contribute and suggest such description for
> this trivial (that may break non-trivial things) patch. So we can
> please you and others in the future.

As I am complaining that I don't know what the submitter was thinking that sounds particularly odd to me. How I am supposed to describe for the group what the commit's author was thinking in a commit message that I would like to see added to a patch when in fact the whole problem is that I don't know specifically what he was thinking? "Changed function to static for no known reason whatsoever" isn't much of a commit message.

-- 
-Drew Northup N1XIM
   AKA RvnPhnx on OPN
________________________________________________
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59