On Mon, Apr 08, 2019 at 02:13:17PM +0900, Junio C Hamano wrote:
> Jeff King <[email protected]> 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
