{"thread":{"id":"54373","subject":"[PATCH] clean up extern decl of functions","startedAt":"2020-10-08T15:28:03Z","lastAt":"2020-10-15T19:30:31Z","messageCount":15,"participants":["Junio C Hamano","Denton Liu","Jeff King","Đoàn Trần Công Danh"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"407155","messageId":"xmqq8scgzqis.fsf@gitster.c.googlers.com","threadId":"54373","inReplyTo":null,"subject":"[PATCH] clean up extern decl of functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-08T15:27:55Z","receivedAt":"2020-10-08T15:28:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Among external function declarations, somehow only these two\nfunctions that return pointer-to-function were declared with\n\"extern\" in front.\n\nIdeally, we should standardise to _have_ explicit \"extern\" in front\nfor all function (and data) decls, but let's make things uniform\nfirst. Bulk re-addition of extern can be done without any extra\ndifficulty with or without this change.\n\nSigned-off-by: Junio C Hamano \n---\n\n * A patch before morning coffee, could be totally off the mark with\n trivial thinko.\n\n git-compat-util.h | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 7a0fb7a045..56e0e1e79d 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -491,9 +491,9 @@ static inline int const_error(void)\n \n void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params));\n void set_error_routine(void (*routine)(const char *err, va_list params));\n-extern void (*get_error_routine(void))(const char *err, va_list params);\n+void (*get_error_routine(void))(const char *err, va_list params);\n void set_warn_routine(void (*routine)(const char *warn, va_list params));\n-extern void (*get_warn_routine(void))(const char *warn, va_list params);\n+void (*get_warn_routine(void))(const char *warn, va_list params);\n void set_die_is_recursing_routine(int (*routine)(void));\n \n int starts_with(const char *str, const char *prefix);\n"},{"id":"407192","messageId":"20201009015505.GB1314@generichostname","threadId":"54373","inReplyTo":"xmqq8scgzqis.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] clean up extern decl of functions","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-10-09T01:55:05Z","receivedAt":"2020-10-09T01:55:10Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Junio,\n\nOn Thu, Oct 08, 2020 at 08:27:55AM -0700, Junio C Hamano wrote:\n> Among external function declarations, somehow only these two\n> functions that return pointer-to-function were declared with\n> \"extern\" in front.\n> \n> Ideally, we should standardise to _have_ explicit \"extern\" in front\n> for all function (and data) decls, but let's make things uniform\n> first. Bulk re-addition of extern can be done without any extra\n> difficulty with or without this change.\n\nWhy are we re-introducing an explicit \"extern\"? Since function decls are\nextern by default, what do we gain by doing this?\n\nYou mentioned in the past[0]\n\n\tI think there is a push to drop the \"extern \" from decls of\n\tfunctions in *.h header files.\n\nso are we reversing that push now?\n\n> Signed-off-by: Junio C Hamano \n\nThe code part looks good to me. Good catch.\n\nThanks,\nDenton\n\n[0]: https://public-inbox.org/git/xmqqef67zz7u.fsf@gitster-ct.c.googlers.com/\n"},{"id":"407194","messageId":"xmqqtuv4uncn.fsf@gitster.c.googlers.com","threadId":"54373","inReplyTo":"20201009015505.GB1314@generichostname","subject":"Re: [PATCH] clean up extern decl of functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-09T02:47:36Z","receivedAt":"2020-10-09T02:47:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> Hi Junio,\n>\n> On Thu, Oct 08, 2020 at 08:27:55AM -0700, Junio C Hamano wrote:\n>> Among external function declarations, somehow only these two\n>> functions that return pointer-to-function were declared with\n>> \"extern\" in front.\n>> \n>> Ideally, we should standardise to _have_ explicit \"extern\" in front\n>> for all function (and data) decls, but let's make things uniform\n>> first. Bulk re-addition of extern can be done without any extra\n>> difficulty with or without this change.\n>\n> Why are we re-introducing an explicit \"extern\"? Since function decls are\n> extern by default, what do we gain by doing this?\n>\n> You mentioned in the past[0]\n>\n> \tI think there is a push to drop the \"extern \" from decls of\n> \tfunctions in *.h header files.\n>\n> so are we reversing that push now?\n\nThat is certainly on the table. Re-read what you quoted and realize\nthat I was not expressing my opinion on the \"push\"; it was just\nstating that other reviewers seem to be in favor.\n\nSee my other response why I think the \"push\" was a bad idea.\n\n"},{"id":"407240","messageId":"xmqqy2kfryiu.fsf@gitster.c.googlers.com","threadId":"54373","inReplyTo":"xmqqtuv4uncn.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] clean up extern decl of functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-09T19:26:49Z","receivedAt":"2020-10-09T19:26:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> Why are we re-introducing an explicit \"extern\"? Since function decls are\n>> extern by default, what do we gain by doing this?\n>>\n>> You mentioned in the past[0]\n>>\n>> \tI think there is a push to drop the \"extern \" from decls of\n>> \tfunctions in *.h header files.\n>>\n>> so are we reversing that push now?\n>\n> That is certainly on the table. Re-read what you quoted and realize\n> that I was not expressing my opinion on the \"push\"; it was just\n> stating that other reviewers seem to be in favor.\n>\n> See my other response why I think the \"push\" was a bad idea.\n\nI'd elaborate a bit more. A proposed update to CodingGuidelines\nwill be sent separately as a follow-up to this message.\n\n\"Are we reversing that push now?\" That is not a question I can\nunilaterally answer yes/no---I do not run dictatorship where I\ncannot survive without telling all the contributors how to cross\ntheir t's and dot their i's. There are things in our coding\nguidelines that tells me to do something differently from how I\nwould, but I can adjust and survive if the primary benefit of having\nguidelines, i.e. making things uniform one way or the other, is net\nwin. When a guideline turns out to be a bad idea, however, I can\npropose to change it. So can you or anybody else ;-)\n\nIn that message, I just told Emily that there is a push to omit\nextern, in the sense that it was the opinion of the prevailing\nlouder voices. Back then, I didn't have an opinion strong enough to\nfavor either way myself, and I was willing to go with the majority\nif many contributors wanted to drop \"extern\" in the hope that it\nwill result in quality code.\n\nBut with the Makefile patch you posted with Dscho's review this\nmorning, it has become apparent to me that it wasn't a great idea\nafter all. It caused you to spend your time to write the RFC patch\nand come up with the regexp, and caused the project to spend\nreviewer bandwidth on the patch.\n\nOf course, how you spend your time is entirely up to you. But if\nyou are going to contribute your time on this project, the project\nappreciates if the time is spent on things that make the codebase\nbetter. And to me, unlike to me \"in the past[0]\", it is reasonably\nclear that the push of omitting \"extern\" ended up wasting resources\nwithout doing much good for the project.\n\nSeeing that the pattern that were trying to be careful didn't catch\nthe decls fixed by the patch you were responding to in this thread\ndid not help improve my impression on the idea of omitting \"extern\".\n\nI think these two decls I touched in this patch were left behind\nwhen somebody, possibly you in b199d714 (*.[ch]: remove extern from\nfunction declarations using sed, 2019-04-29), tried to \"clean up\"\nthe last time, because the pattern used in the conversion did not\ncatch it.\n"},{"id":"407241","messageId":"xmqqtuv3ryhr.fsf_-_@gitster.c.googlers.com","threadId":"54373","inReplyTo":"xmqqy2kfryiu.fsf@gitster.c.googlers.com","subject":"[RFC] CodingGuidelines: mark external declarations with \"extern\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-09T19:27:28Z","receivedAt":"2020-10-09T19:27:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The document recommends not to write \"extern\" in front of a function\ndeclaration, with justification that a decl by default is extern.\n\nWhile it is true that by default decls are extern unless marked with\n\"static\", it does not justify the choice. It only justifies why we\ncould omit it if we wanted to, but does not say anything about the\nreason why we would want to omit it in the first place.\n\nRecently we saw that past mechanical conversion attempts kept a few\nfunction decls with \"extern\" in front. It seems that it was left\nbecause the mechanical conversion tried not to touch external\ndeclaration of pointers to functions, but the pattern used was\nfaulty and excluded functions that return pointers to functions.\n\nFor example, a pointer to a function may look like this:\n\n extern void (*default_frotz_handler)(int frotz);\n\nWe must not omit \"extern\" from this decl, which says \"There exists a\nvariable whose name is default_frotz_handler, which points at a\nfunction that takes an integer and returns nothing.\" It is not a\nfunction declaration and if written without \"extern\" in front,\nrequires the \"common\" extension to be correctly built.\n\nBut a function that returns a pointer to a function looks similar:\n\n extern void (*get_error_routine(void))(const char *message, ...);\n\nwhich says \"There is a get_error_routine() function that takes no\nparameters, and it returns a pointer to a function that takes these\nparameters and returns nothing\".\n\nThe current rule tells us not to write \"extern\" in front, but the\nearlier mechanical conversion missed it. People when writing would\nalso miss it unless they are careful.\n\nInstead of forcing contributors to spend time on on thinking if they\nshould or should not write \"extern\" in front of each of their decl\nwhen they write new code, tell them that our external declarations\nalways say \"extern\" in front. By doing so, we would also prevent a\nmistake of not writing \"extern\" when we need to (i.e. decls of data\nitems, that are not functions) when less experienced developers try\nto mimic how the existing surrounding declarations are written.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/CodingGuidelines | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex 45465bc0c9..eafe41bec8 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -433,10 +433,12 @@ For C programs:\n - Use Git's gettext wrappers to make the user interface\n translatable. See \"Marking strings for translation\" in po/README.\n \n- - Variables and functions local to a given source file should be marked\n- with \"static\". Variables that are visible to other source files\n- must be declared with \"extern\" in header files. However, function\n- declarations should not use \"extern\", as that is already the default.\n+ - Variables and functions local to a given source file should be\n+ marked with \"static\". Variables that are visible to other source\n+ files must be declared with \"extern\" in header files. External\n+ function declarations should also use \"extern\", while external\n+ function definition should not, to make it easier to visually tell\n+ them apart.\n \n - You can launch gdb around your program using the shorthand GIT_DEBUGGER.\n Run `GIT_DEBUGGER=1 ./bin-wrappers/git foo` to simply use gdb as is, or\n-- \n2.29.0-rc1-92-g713508e020\n\n"},{"id":"407243","messageId":"20201009195701.GA967869@coredump.intra.peff.net","threadId":"54373","inReplyTo":"xmqqtuv3ryhr.fsf_-_@gitster.c.googlers.com","subject":"Re: [RFC] CodingGuidelines: mark external declarations with \"extern\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-09T19:57:01Z","receivedAt":"2020-10-09T19:57:45Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 09, 2020 at 12:27:28PM -0700, Junio C Hamano wrote:\n\n> The document recommends not to write \"extern\" in front of a function\n> declaration, with justification that a decl by default is extern.\n> \n> While it is true that by default decls are extern unless marked with\n> \"static\", it does not justify the choice. It only justifies why we\n> could omit it if we wanted to, but does not say anything about the\n> reason why we would want to omit it in the first place.\n\nTrue, but I think that is perhaps a failing of the document, not of the\noriginal motivation. The reason to have _a_ standard is that it is\nconfusing when some functions say \"extern\" and some do not (and it was a\ncontinued source of review friction when people pointed out\ninconsistencies). But the reason to standardize on omitting it is that\nit clutters the line, making it longer and harder to read.\n\nThe argument for including it is less clear to me. You say below:\n\n> [...]By doing so, we would also prevent a\n> mistake of not writing \"extern\" when we need to (i.e. decls of data\n> items, that are not functions) when less experienced developers try\n> to mimic how the existing surrounding declarations are written.\n\nbut to my recollection that has not been a big problem. And it's one\nthat's usually easily caught by the compiler. A missing \"extern\" on a\nvariable will usually get you a multiple-definition warning at\nlink-time (if you manage to also omit the actual definition you won't\nsee that, though \"make sparse\" will warn that your variable ought to be\nstatic).\n\nSo I have a mild preference to omit it, if we were starting from\nscratch. But starting form where we are now, without \"extern\", my\npreference is even stronger, because I don't want to waste submitter or\nreviewer time switching all of the existing cases.\n\n> But a function that returns a pointer to a function looks similar:\n> \n> extern void (*get_error_routine(void))(const char *message, ...);\n\nIMHO the real problem here is that C's syntax for returning a function\npointer is so horrendous. How about this (on top of your earlier patch\nto drop the extern from that declaration)?\n\n-- >8 --\nSubject: [PATCH] usage: define a type for a reporting function\n\nThe usage, die, warning, and error routines all work with a function\npointer that takes the message to be reported. We usually just mention\nthe function's full type inline. But this makes the use of these\npointers hard to read, especially because C's syntax for returning a\nfunction pointer is so awful:\n\n void (*get_error_routine(void))(const char *err, va_list params);\n\nUnless you read it very carefully, this looks like a function pointer\ndeclaration. Let's instead use a single typedef to define a reporting\nfunction, which is the same for all four types.\n\nSigned-off-by: Jeff King \n---\nI wondered if the compiler would complain about attaching a noreturn\nattribute to an already-typedef'd pointer (rather than including it in\nthe typedef). gcc seems to be fine with it, but we could also define two\ntypes, one with noreturn and one without.\n\n git-compat-util.h | 12 +++++++-----\n usage.c | 18 +++++++++---------\n 2 files changed, 16 insertions(+), 14 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 56e0e1e79d..adfea06897 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -489,11 +489,13 @@ static inline int const_error(void)\n #define error_errno(...) (error_errno(__VA_ARGS__), const_error())\n #endif\n \n-void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params));\n-void set_error_routine(void (*routine)(const char *err, va_list params));\n-void (*get_error_routine(void))(const char *err, va_list params);\n-void set_warn_routine(void (*routine)(const char *warn, va_list params));\n-void (*get_warn_routine(void))(const char *warn, va_list params);\n+typedef void (*report_fn)(const char *, va_list params);\n+\n+void set_die_routine(NORETURN_PTR report_fn routine);\n+void set_error_routine(report_fn routine);\n+report_fn get_error_routine(void);\n+void set_warn_routine(report_fn routine);\n+report_fn get_warn_routine(void);\n void set_die_is_recursing_routine(int (*routine)(void));\n \n int starts_with(const char *str, const char *prefix);\ndiff --git a/usage.c b/usage.c\nindex 58fb5fff5f..06665823a2 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -108,33 +108,33 @@ static int die_is_recursing_builtin(void)\n \n /* If we are in a dlopen()ed .so write to a global variable would segfault\n * (ugh), so keep things static. */\n-static NORETURN_PTR void (*usage_routine)(const char *err, va_list params) = usage_builtin;\n-static NORETURN_PTR void (*die_routine)(const char *err, va_list params) = die_builtin;\n-static void (*error_routine)(const char *err, va_list params) = error_builtin;\n-static void (*warn_routine)(const char *err, va_list params) = warn_builtin;\n+static NORETURN_PTR report_fn usage_routine = usage_builtin;\n+static NORETURN_PTR report_fn die_routine = die_builtin;\n+static report_fn error_routine = error_builtin;\n+static report_fn warn_routine = warn_builtin;\n static int (*die_is_recursing)(void) = die_is_recursing_builtin;\n \n-void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params))\n+void set_die_routine(NORETURN_PTR report_fn routine)\n {\n \tdie_routine = routine;\n }\n \n-void set_error_routine(void (*routine)(const char *err, va_list params))\n+void set_error_routine(report_fn routine)\n {\n \terror_routine = routine;\n }\n \n-void (*get_error_routine(void))(const char *err, va_list params)\n+report_fn get_error_routine(void)\n {\n \treturn error_routine;\n }\n \n-void set_warn_routine(void (*routine)(const char *warn, va_list params))\n+void set_warn_routine(report_fn routine)\n {\n \twarn_routine = routine;\n }\n \n-void (*get_warn_routine(void))(const char *warn, va_list params)\n+report_fn get_warn_routine(void)\n {\n \treturn warn_routine;\n }\n-- \n2.29.0.rc1.559.g850010a3b3\n\n"},{"id":"407245","messageId":"xmqqpn5rrvfg.fsf@gitster.c.googlers.com","threadId":"54373","inReplyTo":"20201009195701.GA967869@coredump.intra.peff.net","subject":"Re: [RFC] CodingGuidelines: mark external declarations with \"extern\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-09T20:33:39Z","receivedAt":"2020-10-09T20:33:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The argument for including it is less clear to me. You say below:\n>\n>> [...]By doing so, we would also prevent a\n>> mistake of not writing \"extern\" when we need to (i.e. decls of data\n>> items, that are not functions) when less experienced developers try\n>> to mimic how the existing surrounding declarations are written.\n>\n> but to my recollection that has not been a big problem. And it's one\n> that's usually easily caught by the compiler. A missing \"extern\" on a\n> variable will usually get you a multiple-definition warning at\n> link-time (if you manage to also omit the actual definition you won't\n> see that, though \"make sparse\" will warn that your variable ought to be\n> static).\n\nNot really, that is where the \"common\" extension comes in, to help\nus with it hurt others without it, unknowingly X-<.\n\n $ cat >a.c <<\\EOF\n #include \n #include \"c.h\"\n\n int common = 47;\n\n int main(int ac, char **av)\n {\n printf(\"%d\\n\", common + other);\n return 0;\n }\n EOF\n $ cat >b.c <<\\EOF\n #include \"c.h\"\n\n int other = 22;\n EOF\n $ cat >c.h <<\\EOF\n int common;\n int other;\n EOF\n $ gcc -Wall -o c a.c b.c; ./c\n 59\n\nAnd I have a strong preference, after thinking about it, to have\n\"extern\" in front in the declarations. It gives another clue for\npatterns I feed to \"git grep\" to latch onto, and help my eyes to\nscan and tell decls and defns apart in the output. The benefit\nalone is worth the extra 7 columns in front spent, which you call\n\"clutter\".\n\n> IMHO the real problem here is that C's syntax for returning a function\n> pointer is so horrendous. How about this (on top of your earlier patch\n> to drop the extern from that declaration)?\n\nIn general, I like a typedef for callback function that shortens the\ndecl of a function that takes such a callback, so I think\n\n> +void set_error_routine(report_fn routine);\n> +void set_warn_routine(report_fn routine);\n> +report_fn get_error_routine(void);\n> +report_fn get_warn_routine(void);\n\nthese are good, but they are better with \"extern\" in front in a\nheader file to make it clear they are declarations and not\ndefinitions when they appear in \"git grep\" output.\n"},{"id":"407254","messageId":"20201009230033.GA31120@generichostname","threadId":"54373","inReplyTo":"xmqqpn5rrvfg.fsf@gitster.c.googlers.com","subject":"Re: [RFC] CodingGuidelines: mark external declarations with \"extern\"","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-10-09T23:00:33Z","receivedAt":"2020-10-09T23:00:39Z","isPatch":false,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Junio,\n\nOn Fri, Oct 09, 2020 at 01:33:39PM -0700, Junio C Hamano wrote:\n> Jeff King writes:\n> \n> > The argument for including it is less clear to me. You say below:\n> >\n> >> [...]By doing so, we would also prevent a\n> >> mistake of not writing \"extern\" when we need to (i.e. decls of data\n> >> items, that are not functions) when less experienced developers try\n> >> to mimic how the existing surrounding declarations are written.\n> >\n> > but to my recollection that has not been a big problem. And it's one\n> > that's usually easily caught by the compiler. A missing \"extern\" on a\n> > variable will usually get you a multiple-definition warning at\n> > link-time (if you manage to also omit the actual definition you won't\n> > see that, though \"make sparse\" will warn that your variable ought to be\n> > static).\n> \n> Not really, that is where the \"common\" extension comes in, to help\n> us with it hurt others without it, unknowingly X-<.\n\nI'm not really sure what you mean by the \"common\" extension. \n\n> $ cat >a.c <<\\EOF\n> #include \n> #include \"c.h\"\n> \n> int common = 47;\n> \n> int main(int ac, char **av)\n> {\n> printf(\"%d\\n\", common + other);\n> return 0;\n> }\n> EOF\n> $ cat >b.c <<\\EOF\n> #include \"c.h\"\n> \n> int other = 22;\n> EOF\n> $ cat >c.h <<\\EOF\n> int common;\n> int other;\n> EOF\n> $ gcc -Wall -o c a.c b.c; ./c\n> 59\n\nOn gcc 10.2.0, it errors out successfully. Although on clang 10.0.1, it\ncompiles successfully and produces \"69\". That being said, I think extern\nvariables are relatively rare in our codebase and, when it happens, they\nusually come as part of lists of other extern variables so a developer\nwho's mimicking the surrounding code would be able to copy it\nsuccessfully. Otherwise, the decl usually pops out in header files as it\nis quite unusual.\n\n> And I have a strong preference, after thinking about it, to have\n> \"extern\" in front in the declarations. It gives another clue for\n> patterns I feed to \"git grep\" to latch onto, and help my eyes to\n> scan and tell decls and defns apart in the output. The benefit\n> alone is worth the extra 7 columns in front spent, which you call\n> \"clutter\".\n\nTo be honest, I do not have any preference between having the explicit\nextern or not. I do have a strong preference, however, for having a\ncodebase that's consistently written. When I was doing the refactor, I\nwouldn't have minded introducing extern everywhere although that wasn't\nsuggested as an alternative.\n\nI agree that these are all benefits of declaring functions explicitly as\nextern. However, I don't think they're worth the cost of either another\nhuge rewrite or an inconsistent codebase.\n\n> > IMHO the real problem here is that C's syntax for returning a function\n> > pointer is so horrendous. How about this (on top of your earlier patch\n> > to drop the extern from that declaration)?\n> \n> In general, I like a typedef for callback function that shortens the\n> decl of a function that takes such a callback, so I think\n> \n> > +void set_error_routine(report_fn routine);\n> > +void set_warn_routine(report_fn routine);\n> > +report_fn get_error_routine(void);\n> > +report_fn get_warn_routine(void);\n> \n> these are good, but they are better with \"extern\" in front in a\n> header file to make it clear they are declarations and not\n> definitions when they appear in \"git grep\" output.\n\nI agree that this looks a lot better, with or without the extern.\n"},{"id":"407255","messageId":"xmqqblhbrodw.fsf@gitster.c.googlers.com","threadId":"54373","inReplyTo":"20201009230033.GA31120@generichostname","subject":"Re: [RFC] CodingGuidelines: mark external declarations with \"extern\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-09T23:05:47Z","receivedAt":"2020-10-09T23:05:54Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n>> And I have a strong preference, after thinking about it, to have\n>> \"extern\" in front in the declarations. It gives another clue for\n>> patterns I feed to \"git grep\" to latch onto, and help my eyes to\n>> scan and tell decls and defns apart in the output. The benefit\n>> alone is worth the extra 7 columns in front spent, which you call\n>> \"clutter\".\n>\n> To be honest, I do not have any preference between having the explicit\n> extern or not. I do have a strong preference, however, for having a\n> codebase that's consistently written. When I was doing the refactor, I\n> wouldn't have minded introducing extern everywhere although that wasn't\n> suggested as an alternative.\n>\n> I agree that these are all benefits of declaring functions explicitly as\n> extern. However, I don't think they're worth the cost of either another\n> huge rewrite or an inconsistent codebase.\n\nYes, there is a cost associated with having made a mistake in the\npast. Biting the bullet now, perhaps as the first tree-wide change\nimmediately after the upcoming release, while the tree is quiescent,\nwould help us in the longer term, than having to live without extern\non declarations.\n"},{"id":"407256","messageId":"20201010003701.GC27795@danh.dev","threadId":"54373","inReplyTo":"xmqqpn5rrvfg.fsf@gitster.c.googlers.com","subject":"Re: [RFC] CodingGuidelines: mark external declarations with \"extern\"","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-10-10T00:37:01Z","receivedAt":"2020-10-10T00:56:15Z","isPatch":false,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-10-09 13:33:39-0700, Junio C Hamano wrote:\n> Jeff King writes:\n> \n> > The argument for including it is less clear to me. You say below:\n> >\n> >> [...]By doing so, we would also prevent a\n> >> mistake of not writing \"extern\" when we need to (i.e. decls of data\n> >> items, that are not functions) when less experienced developers try\n> >> to mimic how the existing surrounding declarations are written.\n> >\n> > but to my recollection that has not been a big problem. And it's one\n> > that's usually easily caught by the compiler. A missing \"extern\" on a\n> > variable will usually get you a multiple-definition warning at\n> > link-time (if you manage to also omit the actual definition you won't\n> > see that, though \"make sparse\" will warn that your variable ought to be\n> > static).\n> \n> Not really, that is where the \"common\" extension comes in, to help\n> us with it hurt others without it, unknowingly X-<.\n\nYes, that's where tentative definition jumpes in.\nBut, tentative definition is known to cause headache to compiler optimization.\nAnd from GCC 10, gcc change to `-fno-common` by default.\n\nWe can enable `-fno-common` now if we can detect our compiler is gcc,\nbut, I don't think it worth to fiddle with Makefile to add that logic.\n\n> \n> $ cat >a.c <<\\EOF\n> #include \n> #include \"c.h\"\n> \n> int common = 47;\n> \n> int main(int ac, char **av)\n> {\n> printf(\"%d\\n\", common + other);\n> return 0;\n> }\n> EOF\n> $ cat >b.c <<\\EOF\n> #include \"c.h\"\n> \n> int other = 22;\n> EOF\n> $ cat >c.h <<\\EOF\n> int common;\n> int other;\n> EOF\n> $ gcc -Wall -o c a.c b.c; ./c\n> 59\n> \n> And I have a strong preference, after thinking about it, to have\n> \"extern\" in front in the declarations. It gives another clue for\n> patterns I feed to \"git grep\" to latch onto, and help my eyes to\n> scan and tell decls and defns apart in the output.\n\n\nWith this argument, I think adding \"extern\" is worth it.\nIt could help people find the code better.\n\n-- \nDanh\n"},{"id":"407559","messageId":"20201015013607.GA388244@coredump.intra.peff.net","threadId":"54373","inReplyTo":"xmqqpn5rrvfg.fsf@gitster.c.googlers.com","subject":"Re: [RFC] CodingGuidelines: mark external declarations with \"extern\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-15T01:36:07Z","receivedAt":"2020-10-15T01:36:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 09, 2020 at 01:33:39PM -0700, Junio C Hamano wrote:\n\n> >> [...]By doing so, we would also prevent a\n> >> mistake of not writing \"extern\" when we need to (i.e. decls of data\n> >> items, that are not functions) when less experienced developers try\n> >> to mimic how the existing surrounding declarations are written.\n> >\n> > but to my recollection that has not been a big problem. And it's one\n> > that's usually easily caught by the compiler. A missing \"extern\" on a\n> > variable will usually get you a multiple-definition warning at\n> > link-time (if you manage to also omit the actual definition you won't\n> > see that, though \"make sparse\" will warn that your variable ought to be\n> > static).\n> \n> Not really, that is where the \"common\" extension comes in, to help\n> us with it hurt others without it, unknowingly X-<.\n\nAs others noted, gcc 10 actually does complain about this. And we can\neasily stick -fno-common into the DEVELOPER knobs, if it's something we\nwant to catch (I had actually forgotten it wasn't the default).\n\nThat said....\n\n> And I have a strong preference, after thinking about it, to have\n> \"extern\" in front in the declarations. It gives another clue for\n> patterns I feed to \"git grep\" to latch onto, and help my eyes to\n> scan and tell decls and defns apart in the output. The benefit\n> alone is worth the extra 7 columns in front spent, which you call\n> \"clutter\".\n\nI still don't like it, and I'm convinced spending any effort to switch\nbetween the two styles is a waste of time. But it's absolutely not the\nhill I want to die on, so if you feel strongly, go for it.\n\n> > +void set_error_routine(report_fn routine);\n> > +void set_warn_routine(report_fn routine);\n> > +report_fn get_error_routine(void);\n> > +report_fn get_warn_routine(void);\n> \n> these are good, but they are better with \"extern\" in front in a\n> header file to make it clear they are declarations and not\n> definitions when they appear in \"git grep\" output.\n\nI see you picked up my patch as jk/report-fn-typedef, but applied it\ndirectly on v2.28, and not on top of your \"drop these extra externs\"\npatch. That makes sense if we're not going to remove them, but then your\nconflict resolution shows my patch as removing them. :)\n\nIf we're going to keep them, it should probably leave them in the\nexisting spots? Or I guess it is OK as-is if you're planning to add them\nback in to all of the functions shortly after, not just the ones that\nalready had extern on them.\n\n-Peff\n"},{"id":"407631","messageId":"xmqq8sc7h0ld.fsf@gitster.c.googlers.com","threadId":"54373","inReplyTo":"20201015013607.GA388244@coredump.intra.peff.net","subject":"Re: [RFC] CodingGuidelines: mark external declarations with \"extern\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-15T17:15:42Z","receivedAt":"2020-10-15T17:15:49Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Oct 09, 2020 at 01:33:39PM -0700, Junio C Hamano wrote:\n>\n>> >> [...]By doing so, we would also prevent a\n>> >> mistake of not writing \"extern\" when we need to (i.e. decls of data\n>> >> items, that are not functions) when less experienced developers try\n>> >> to mimic how the existing surrounding declarations are written.\n>> >\n>> > but to my recollection that has not been a big problem. And it's one\n>> > that's usually easily caught by the compiler. A missing \"extern\" on a\n>> > variable will usually get you a multiple-definition warning at\n>> > link-time (if you manage to also omit the actual definition you won't\n>> > see that, though \"make sparse\" will warn that your variable ought to be\n>> > static).\n>> \n>> Not really, that is where the \"common\" extension comes in, to help\n>> us with it hurt others without it, unknowingly X-<.\n>\n> As others noted, gcc 10 actually does complain about this. And we can\n> easily stick -fno-common into the DEVELOPER knobs, if it's something we\n> want to catch (I had actually forgotten it wasn't the default).\n\nYup, that is a good thing to do regardless. I am mostly interested\nin seeing \"extern\" in front of all extern decls (not defns) from\nhuman readers' point of view, though.\n\n> I see you picked up my patch as jk/report-fn-typedef, but applied it\n> directly on v2.28, and not on top of your \"drop these extra externs\"\n> patch. That makes sense if we're not going to remove them, but then your\n> conflict resolution shows my patch as removing them. :)\n\nI think the patch rearranged that way shows why the new typedef\nshines.\n\nIt makes it immediately recognisable that set_die_routine() and\nfriends are functions, not pointers to functions, and because the\nreason why \"extern\" was in front of them was because the last person\nwho \"cleaned up\" the header failed to recoginise that they are\nfunctions without the help from this new typedef.\n\nAll of that depends on the fact that everybody understands that the\ncurrent rule is not to write \"extern\" in front of external\ndeclaration of functions, so perhaps we may want to update the log\nmessage to mention why \"extern\" were removed from these two.\n\nThanks.\n"},{"id":"407662","messageId":"20201015192845.GD1108210@coredump.intra.peff.net","threadId":"54373","inReplyTo":"xmqq8sc7h0ld.fsf@gitster.c.googlers.com","subject":"Re: [RFC] CodingGuidelines: mark external declarations with \"extern\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-15T19:28:45Z","receivedAt":"2020-10-15T19:28:49Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 15, 2020 at 10:15:42AM -0700, Junio C Hamano wrote:\n\n> > As others noted, gcc 10 actually does complain about this. And we can\n> > easily stick -fno-common into the DEVELOPER knobs, if it's something we\n> > want to catch (I had actually forgotten it wasn't the default).\n> \n> Yup, that is a good thing to do regardless. I am mostly interested\n> in seeing \"extern\" in front of all extern decls (not defns) from\n> human readers' point of view, though.\n\nOK, patch incoming.\n\n> > I see you picked up my patch as jk/report-fn-typedef, but applied it\n> > directly on v2.28, and not on top of your \"drop these extra externs\"\n> > patch. That makes sense if we're not going to remove them, but then your\n> > conflict resolution shows my patch as removing them. :)\n> \n> I think the patch rearranged that way shows why the new typedef\n> shines.\n> \n> It makes it immediately recognisable that set_die_routine() and\n> friends are functions, not pointers to functions, and because the\n> reason why \"extern\" was in front of them was because the last person\n> who \"cleaned up\" the header failed to recoginise that they are\n> functions without the help from this new typedef.\n> \n> All of that depends on the fact that everybody understands that the\n> current rule is not to write \"extern\" in front of external\n> declaration of functions, so perhaps we may want to update the log\n> message to mention why \"extern\" were removed from these two.\n\nI added:\n\n Note that this also removes the \"extern\" from these declarations to\n match the surrounding functions. They were missed in 554544276a\n (*.[ch]: remove extern from function declarations using spatch,\n 2019-04-29) presumably because of the unusual syntax.\n\nwhich I think clarifies it.\n\nHere's that patch re-rolled, plus the DEVELOPER one.\n\n [1/2]: usage: define a type for a reporting function\n [2/2]: config.mak.dev: build with -fno-common\n\n config.mak.dev | 1 +\n git-compat-util.h | 12 +++++++-----\n usage.c | 18 +++++++++---------\n 3 files changed, 17 insertions(+), 14 deletions(-)\n\n-Peff\n"},{"id":"407663","messageId":"20201015193004.GA1129761@coredump.intra.peff.net","threadId":"54373","inReplyTo":"20201015192845.GD1108210@coredump.intra.peff.net","subject":"[PATCH v2 1/2] usage: define a type for a reporting function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-15T19:30:04Z","receivedAt":"2020-10-15T19:30:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The usage, die, warning, and error routines all work with a function\npointer that takes the message to be reported. We usually just mention\nthe function's full type inline. But this makes the use of these\npointers hard to read, especially because C's syntax for returning a\nfunction pointer is so awful:\n\n void (*get_error_routine(void))(const char *err, va_list params);\n\nUnless you read it very carefully, this looks like a function pointer\ndeclaration. Let's instead use a single typedef to define a reporting\nfunction, which is the same for all four types.\n\nNote that this also removes the \"extern\" from these declarations to\nmatch the surrounding functions. They were missed in 554544276a (*.[ch]:\nremove extern from function declarations using spatch, 2019-04-29)\npresumably because of the unusual syntax.\n\nSigned-off-by: Jeff King \n---\nThis replaces the tip of jk/report-fn-typedef.\n\n git-compat-util.h | 12 +++++++-----\n usage.c | 18 +++++++++---------\n 2 files changed, 16 insertions(+), 14 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 7a0fb7a045..adfea06897 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -489,11 +489,13 @@ static inline int const_error(void)\n #define error_errno(...) (error_errno(__VA_ARGS__), const_error())\n #endif\n \n-void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params));\n-void set_error_routine(void (*routine)(const char *err, va_list params));\n-extern void (*get_error_routine(void))(const char *err, va_list params);\n-void set_warn_routine(void (*routine)(const char *warn, va_list params));\n-extern void (*get_warn_routine(void))(const char *warn, va_list params);\n+typedef void (*report_fn)(const char *, va_list params);\n+\n+void set_die_routine(NORETURN_PTR report_fn routine);\n+void set_error_routine(report_fn routine);\n+report_fn get_error_routine(void);\n+void set_warn_routine(report_fn routine);\n+report_fn get_warn_routine(void);\n void set_die_is_recursing_routine(int (*routine)(void));\n \n int starts_with(const char *str, const char *prefix);\ndiff --git a/usage.c b/usage.c\nindex 58fb5fff5f..06665823a2 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -108,33 +108,33 @@ static int die_is_recursing_builtin(void)\n \n /* If we are in a dlopen()ed .so write to a global variable would segfault\n * (ugh), so keep things static. */\n-static NORETURN_PTR void (*usage_routine)(const char *err, va_list params) = usage_builtin;\n-static NORETURN_PTR void (*die_routine)(const char *err, va_list params) = die_builtin;\n-static void (*error_routine)(const char *err, va_list params) = error_builtin;\n-static void (*warn_routine)(const char *err, va_list params) = warn_builtin;\n+static NORETURN_PTR report_fn usage_routine = usage_builtin;\n+static NORETURN_PTR report_fn die_routine = die_builtin;\n+static report_fn error_routine = error_builtin;\n+static report_fn warn_routine = warn_builtin;\n static int (*die_is_recursing)(void) = die_is_recursing_builtin;\n \n-void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params))\n+void set_die_routine(NORETURN_PTR report_fn routine)\n {\n \tdie_routine = routine;\n }\n \n-void set_error_routine(void (*routine)(const char *err, va_list params))\n+void set_error_routine(report_fn routine)\n {\n \terror_routine = routine;\n }\n \n-void (*get_error_routine(void))(const char *err, va_list params)\n+report_fn get_error_routine(void)\n {\n \treturn error_routine;\n }\n \n-void set_warn_routine(void (*routine)(const char *warn, va_list params))\n+void set_warn_routine(report_fn routine)\n {\n \twarn_routine = routine;\n }\n \n-void (*get_warn_routine(void))(const char *warn, va_list params)\n+report_fn get_warn_routine(void)\n {\n \treturn warn_routine;\n }\n-- \n2.29.0.rc1.562.g7bd9bc8902\n\n"},{"id":"407664","messageId":"20201015193029.GB1129761@coredump.intra.peff.net","threadId":"54373","inReplyTo":"20201015192845.GD1108210@coredump.intra.peff.net","subject":"[PATCH v2 2/2] config.mak.dev: build with -fno-common","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-15T19:30:29Z","receivedAt":"2020-10-15T19:30:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It's an easy mistake to define a variable in a header with \"int x;\" when\nyou really meant to only declare the variable as \"extern int x;\"\ninstead. Clang and gcc will both allow this when building with\n\"-fcommon\"; they put these \"tentative definitions\" in a common block\nwhich the linker is able to resolve.\n\nThis is the default in clang and was the default in gcc until gcc-10,\nsince it helps some legacy code. However, we would prefer not to rely on\nthis because:\n\n - using \"extern\" makes the intent more clear (so it's a style issue,\n but it's one the compiler can help us catch)\n\n - according to the gcc manpage, it may yield a speed and code size\n penalty\n\nSo let's build explicitly with -fno-common when the DEVELOPER knob is\nset, which will let developers using clang and older versions of gcc\nnotice these problems.\n\nI didn't bother making this conditional on a particular version of gcc.\nAs far as I know, this option has been available forever in both gcc and\nclang, so old versions don't need to avoid it. And we already expect gcc\nand clang options throughout config.mak.dev, so it's unlikely anybody\nsetting the DEVELOPER knob is using anything else. It's a noop on\ngcc-10, of course, but it's not worth trying to exclude it there.\n\nNote that there's nothing to fix in the code; we already don't have any\nissues here. But if you want to test the patch, you can add a bare \"int\nx;\" into cache.h, which will cause the link step to fail.\n\nSigned-off-by: Jeff King \n---\nNew in this version. It could be queued separately from the first.\n\n config.mak.dev | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/config.mak.dev b/config.mak.dev\nindex 89b218d11a..89fefacd94 100644\n--- a/config.mak.dev\n+++ b/config.mak.dev\n@@ -15,6 +15,7 @@ DEVELOPER_CFLAGS += -Wpointer-arith\n DEVELOPER_CFLAGS += -Wstrict-prototypes\n DEVELOPER_CFLAGS += -Wunused\n DEVELOPER_CFLAGS += -Wvla\n+DEVELOPER_CFLAGS += -fno-common\n \n ifndef COMPILER_FEATURES\n COMPILER_FEATURES := $(shell ./detect-compiler $(CC))\n-- \n2.29.0.rc1.562.g7bd9bc8902\n"}]}