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

[Jeff King <peff@peff.net>]

On Fri, Oct 09, 2020 at 01:33:39PM -0700, Junio C Hamano wrote:

> >> [...]By doing so, we would also prevent a
> >> mistake of not writing "extern" when we need to (i.e. decls of data
> >> items, that are not functions) when less experienced developers try
> >> to mimic how the existing surrounding declarations are written.
> >
> > but to my recollection that has not been a big problem. And it's one
> > that's usually easily caught by the compiler. A missing "extern" on a
> > variable will usually get you a multiple-definition warning at
> > link-time (if you manage to also omit the actual definition you won't
> > see that, though "make sparse" will warn that your variable ought to be
> > static).
> 
> Not really, that is where the "common" extension comes in, to help
> us with it hurt others without it, unknowingly X-<.

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

That said....

> And I have a strong preference, after thinking about it, to have
> "extern" in front in the declarations.  It gives another clue for
> patterns I feed to "git grep" to latch onto, and help my eyes to
> scan and tell decls and defns apart in the output.  The benefit
> alone is worth the extra 7 columns in front spent, which you call
> "clutter".

I still don't like it, and I'm convinced spending any effort to switch
between the two styles is a waste of time. But it's absolutely not the
hill I want to die on, so if you feel strongly, go for it.

> > +void set_error_routine(report_fn routine);
> > +void set_warn_routine(report_fn routine);
> > +report_fn get_error_routine(void);
> > +report_fn get_warn_routine(void);
> 
> these are good, but they are better with "extern" in front in a
> header file to make it clear they are declarations and not
> definitions when they appear in "git grep" output.

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. :)

If we're going to keep them, it should probably leave them in the
existing spots? Or I guess it is OK as-is if you're planning to add them
back in to all of the functions shortly after, not just the ones that
already had extern on them.

-Peff