git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jeff King <peff@peff.net>
Cc: git@vger.kernel.org, Denton Liu <liu.denton@gmail.com>
Subject: Re: [RFC] CodingGuidelines: mark external declarations with "extern"
Date: Thu, 15 Oct 2020 10:15:42 -0700	[thread overview]
Message-ID: <xmqq8sc7h0ld.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20201015013607.GA388244@coredump.intra.peff.net> (Jeff King's message of "Wed, 14 Oct 2020 21:36:07 -0400")

Jeff King <peff@peff.net> writes:

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

  reply	other threads:[~2020-10-15 17:15 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-08 15:27 [PATCH] clean up extern decl of functions Junio C Hamano
2020-10-09  1:55 ` Denton Liu
2020-10-09  2:47   ` Junio C Hamano
2020-10-09 19:26     ` Junio C Hamano
2020-10-09 19:27       ` [RFC] CodingGuidelines: mark external declarations with "extern" Junio C Hamano
2020-10-09 19:57         ` Jeff King
2020-10-09 20:33           ` Junio C Hamano
2020-10-09 23:00             ` Denton Liu
2020-10-09 23:05               ` Junio C Hamano
2020-10-10  0:37             ` Đoàn Trần Công Danh
2020-10-15  1:36             ` Jeff King
2020-10-15 17:15               ` Junio C Hamano [this message]
2020-10-15 19:28                 ` Jeff King
2020-10-15 19:30                   ` [PATCH v2 1/2] usage: define a type for a reporting function Jeff King
2020-10-15 19:30                   ` [PATCH v2 2/2] config.mak.dev: build with -fno-common Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq8sc7h0ld.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=liu.denton@gmail.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).