Re: [PATCH v2 01/13] packfile.h: drop extern from function declarations

From: Jeff King <peff@peff.net>
Date: Apr 8, 2019, 20:32 UTC
Message-ID: <20190408203252.GA5696@sigill.intra.peff.net>
In-Reply-To: <xmqqzhp19j0y.fsf@gitster-ct.c.googlers.com>

On Mon, Apr 08, 2019 at 02:13:17PM +0900, Junio C Hamano wrote:

Show 23 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > On Fri, Apr 05, 2019 at 08:19:30PM +0100, Ramsay Jones wrote:
> >
> >> >  /* global flag to enable extra checks when accessing packed objects */
> >> > -extern int do_check_packed_object_crc;
> >> > +int do_check_packed_object_crc;
> >> 
> >> ... removing this 'extern' on an int variable sends 'sparse'
> >> into a frenzy of warnings! :-D
> >> 
> >> [You didn't use a global s/extern// by any chance?]
> >
> > Oh my. I did look at each one, but probably via replace-and-confirm in
> > vim. I don't know how I managed to botch that one so badly.
> 
> Perhaps we should keep 'extern' even when declaring (not defining) a
> public function in the header file to avoid a gotcha like this?
> 
> What was the reasoning behind the insn in CodingGuidelines?  "As it
> is already the default" does qualify as a reasonable justification
> for telling "extern is not needed for functions" to our readers, but
> not quite enough for "extern should not be used for functions".

I think the reasoning is just that it's useless noise, so it makes the resulting lines longer (which are often already too-long) and harder to read.

For this particular patch, I don't care that much about the existing functions, which I'm not touching, but rather was adding a new one. And my options were:

  - use "extern" there to match; even if we don't want to go through the
    code churn of cleaning up existing cases, I think we shouldn't be
    encouraging it in new ones. Even more crazy to me would actually
    be review telling somebody to add an extern.

  - intermingle it with the existing extern ones. Low risk, but leaves
    people wondering why some have extern and some do not.

  - clean up the existing cases first

I dunno. Like all code churn, these kinds of clean-ups have the possibility of accidentally screwing something up. But they are at least a one-time pain, as long as we do not keep changing our mind back and forth.

-Peff