Re: [RFC] CodingGuidelines: mark external declarations with "extern"

From: Jeff King <peff@peff.net>
Date: Oct 15, 2020, 19:28 UTC
Message-ID: <20201015192845.GD1108210@coredump.intra.peff.net>
In-Reply-To: <xmqq8sc7h0ld.fsf@gitster.c.googlers.com>

On Thu, Oct 15, 2020 at 10:15:42AM -0700, Junio C Hamano wrote:

Show 7 quoted lines

> > As others noted, gcc 10 actually does complain about this. And we can
> > easily stick -fno-common into the DEVELOPER knobs, if it's something we
> > want to catch (I had actually forgotten it wasn't the default).
> 
> Yup, that is a good thing to do regardless.  I am mostly interested
> in seeing "extern" in front of all extern decls (not defns) from
> human readers' point of view, though.

OK, patch incoming.

Show 18 quoted lines

> > I see you picked up my patch as jk/report-fn-typedef, but applied it
> > directly on v2.28, and not on top of your "drop these extra externs"
> > patch. That makes sense if we're not going to remove them, but then your
> > conflict resolution shows my patch as removing them. :)
> 
> I think the patch rearranged that way shows why the new typedef
> shines.
> 
> It makes it immediately recognisable that set_die_routine() and
> friends are functions, not pointers to functions, and because the
> reason why "extern" was in front of them was because the last person
> who "cleaned up" the header failed to recoginise that they are
> functions without the help from this new typedef.
> 
> All of that depends on the fact that everybody understands that the
> current rule is not to write "extern" in front of external
> declaration of functions, so perhaps we may want to update the log
> message to mention why "extern" were removed from these two.

I added:

  Note that this also removes the "extern" from these declarations to
  match the surrounding functions. They were missed in 554544276a
  (*.[ch]: remove extern from function declarations using spatch,
  2019-04-29) presumably because of the unusual syntax.

which I think clarifies it.

Here's that patch re-rolled, plus the DEVELOPER one.

  [1/2]: usage: define a type for a reporting function
  [2/2]: config.mak.dev: build with -fno-common

 config.mak.dev    |  1 +
 git-compat-util.h | 12 +++++++-----
 usage.c           | 18 +++++++++---------
 3 files changed, 17 insertions(+), 14 deletions(-)

-Peff