git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jeff King <peff@peff.net>
Date
Oct 15, 2020, 01:36 UTC
Message-ID
<20201015013607.GA388244@coredump.intra.peff.net>
In-Reply-To
<xmqqpn5rrvfg.fsf@gitster.c.googlers.com>
On Fri, Oct 09, 2020 at 01:33:39PM -0700, Junio C Hamano wrote:
Show 14 quoted lines
> >> [...]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....
Show 6 quoted lines
> 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.

Show 8 quoted lines
> > +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
Previous: Đoàn Trần Công DanhNext: Junio C Hamano
Message 11 of 15 in “clean up extern decl of functions”
  1. clean up extern decl of functionsJunio C Hamano, Oct 8, 2020
  2. Denton LiuOct 9, 2020
  3. Junio C HamanoOct 9, 2020
  4. Junio C HamanoOct 9, 2020
  5. [RFC] CodingGuidelines: mark external declarations with "extern"Junio C Hamano, Oct 9, 2020
  6. Jeff KingOct 9, 2020
  7. Junio C HamanoOct 9, 2020
  8. Denton LiuOct 9, 2020
  9. Junio C HamanoOct 9, 2020
  10. Đoàn Trần Công DanhOct 10, 2020
  11. Jeff KingOct 15, 2020
  12. Junio C HamanoOct 15, 2020
  13. Jeff KingOct 15, 2020
  14. 1/2 usage: define a type for a reporting functionJeff King, Oct 15, 2020
  15. 2/2 config.mak.dev: build with -fno-commonJeff King, Oct 15, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.