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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 15, 2020, 17:15 UTC
Message-ID: <xmqq8sc7h0ld.fsf@gitster.c.googlers.com>
In-Reply-To: <20201015013607.GA388244@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 20 quoted lines

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

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.

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

Thanks.