git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: git@vger.kernel.org
Cc: Denton Liu <liu.denton@gmail.com>
Subject: [RFC] CodingGuidelines: mark external declarations with "extern"
Date: Fri, 09 Oct 2020 12:27:28 -0700	[thread overview]
Message-ID: <xmqqtuv3ryhr.fsf_-_@gitster.c.googlers.com> (raw)
In-Reply-To: <xmqqy2kfryiu.fsf@gitster.c.googlers.com> (Junio C. Hamano's message of "Fri, 09 Oct 2020 12:26:49 -0700")

The document recommends not to write "extern" in front of a function
declaration, with justification that a decl by default is extern.

While it is true that by default decls are extern unless marked with
"static", it does not justify the choice.  It only justifies why we
could omit it if we wanted to, but does not say anything about the
reason why we would want to omit it in the first place.

Recently we saw that past mechanical conversion attempts kept a few
function decls with "extern" in front.  It seems that it was left
because the mechanical conversion tried not to touch external
declaration of pointers to functions, but the pattern used was
faulty and excluded functions that return pointers to functions.

For example, a pointer to a function may look like this:

    extern void (*default_frotz_handler)(int frotz);

We must not omit "extern" from this decl, which says "There exists a
variable whose name is default_frotz_handler, which points at a
function that takes an integer and returns nothing."  It is not a
function declaration and if written without "extern" in front,
requires the "common" extension to be correctly built.

But a function that returns a pointer to a function looks similar:

    extern void (*get_error_routine(void))(const char *message, ...);

which says "There is a get_error_routine() function that takes no
parameters, and it returns a pointer to a function that takes these
parameters and returns nothing".

The current rule tells us not to write "extern" in front, but the
earlier mechanical conversion missed it.  People when writing would
also miss it unless they are careful.

Instead of forcing contributors to spend time on on thinking if they
should or should not write "extern" in front of each of their decl
when they write new code, tell them that our external declarations
always say "extern" in front.  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.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/CodingGuidelines | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 45465bc0c9..eafe41bec8 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -433,10 +433,12 @@ For C programs:
  - Use Git's gettext wrappers to make the user interface
    translatable. See "Marking strings for translation" in po/README.
 
- - Variables and functions local to a given source file should be marked
-   with "static". Variables that are visible to other source files
-   must be declared with "extern" in header files. However, function
-   declarations should not use "extern", as that is already the default.
+ - Variables and functions local to a given source file should be
+   marked with "static". Variables that are visible to other source
+   files must be declared with "extern" in header files.  External
+   function declarations should also use "extern", while external
+   function definition should not, to make it easier to visually tell
+   them apart.
 
  - You can launch gdb around your program using the shorthand GIT_DEBUGGER.
    Run `GIT_DEBUGGER=1 ./bin-wrappers/git foo` to simply use gdb as is, or
-- 
2.29.0-rc1-92-g713508e020


  reply	other threads:[~2020-10-09 19:27 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       ` Junio C Hamano [this message]
2020-10-09 19:57         ` [RFC] CodingGuidelines: mark external declarations with "extern" 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
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=xmqqtuv3ryhr.fsf_-_@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=liu.denton@gmail.com \
    /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).