{"thread":{"id":"54362","subject":"[PATCH] compat/mingw.h: drop extern from function declaration","startedAt":"2020-10-07T08:32:18Z","lastAt":"2020-10-09T02:45:19Z","messageCount":7,"participants":["Denton Liu","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"407040","messageId":"9e5a8625cab217bd6aaba68da081bc801354b903.1602059508.git.liu.denton@gmail.com","threadId":"54362","inReplyTo":null,"subject":"[PATCH] compat/mingw.h: drop extern from function declaration","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-10-07T08:31:59Z","receivedAt":"2020-10-07T08:32:18Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In 554544276a (*.[ch]: remove extern from function declarations using\nspatch, 2019-04-29), `extern` on function declarations were declared to\nbe redundant and thus removed from the codebase. An `extern` was\naccidentally reintroduced in 08809c09aa (mingw: add a helper function to\nattach GDB to the current process, 2020-02-13).\n\nRemove this spurious `extern`.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n\nNotes:\n    This patch is based on 554544276a, formerly 'js/mingw-open-in-gdb'.\n\n compat/mingw.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex 88c07c4520..5c34547539 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -599,7 +599,7 @@ int main(int argc, const char **argv);\n  * Call this function to open a new MinTTY (this assumes you are in Git for\n  * Windows' SDK) with a GDB that attaches to the current process right away.\n  */\n-extern void open_in_gdb(void);\n+void open_in_gdb(void);\n \n /*\n  * Used by Pthread API implementation for Windows\n-- \n2.28.0.942.g77c4c6094c\n\n"},{"id":"407046","messageId":"nycvar.QRO.7.76.6.2010070844340.50@tvgsbejvaqbjf.bet","threadId":"54362","inReplyTo":"9e5a8625cab217bd6aaba68da081bc801354b903.1602059508.git.liu.denton@gmail.com","subject":"Re: [PATCH] compat/mingw.h: drop extern from function declaration","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-10-07T06:44:45Z","receivedAt":"2020-10-07T11:50:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Denton,\n\nOn Wed, 7 Oct 2020, Denton Liu wrote:\n\n> In 554544276a (*.[ch]: remove extern from function declarations using\n> spatch, 2019-04-29), `extern` on function declarations were declared to\n> be redundant and thus removed from the codebase. An `extern` was\n> accidentally reintroduced in 08809c09aa (mingw: add a helper function to\n> attach GDB to the current process, 2020-02-13).\n>\n> Remove this spurious `extern`.\n\nACK!\n\nThanks,\nDscho\n\n>\n> Signed-off-by: Denton Liu <liu.denton@gmail.com>\n> ---\n>\n> Notes:\n>     This patch is based on 554544276a, formerly 'js/mingw-open-in-gdb'.\n>\n>  compat/mingw.h | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/compat/mingw.h b/compat/mingw.h\n> index 88c07c4520..5c34547539 100644\n> --- a/compat/mingw.h\n> +++ b/compat/mingw.h\n> @@ -599,7 +599,7 @@ int main(int argc, const char **argv);\n>   * Call this function to open a new MinTTY (this assumes you are in Git for\n>   * Windows' SDK) with a GDB that attaches to the current process right away.\n>   */\n> -extern void open_in_gdb(void);\n> +void open_in_gdb(void);\n>\n>  /*\n>   * Used by Pthread API implementation for Windows\n> --\n> 2.28.0.942.g77c4c6094c\n>\n>\n"},{"id":"407123","messageId":"1c2ee1c40ccde18a008262aeaf23a17da2064eae.1602142232.git.liu.denton@gmail.com","threadId":"54362","inReplyTo":"9e5a8625cab217bd6aaba68da081bc801354b903.1602059508.git.liu.denton@gmail.com","subject":"[RFC PATCH] Makefile: create externcheck target","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-10-08T07:30:55Z","receivedAt":"2020-10-08T07:31:14Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In b199d7147a (*.[ch]: remove extern from function declarations using\nsed, 2019-04-29), we used sed to remove extern from function\ndefinitions. In order to help find and remove future instances of this,\nteach Makefile the `externcheck` target which runs the sed script\nincluded in that commit on all applicable source files.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n\nNotes:\n    I run this target periodically to ensure that no new instances of extern\n    function definitions are introduced. Is this something that we want to\n    consider adding for real?\n\n Makefile | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/Makefile b/Makefile\nindex 5311b1d2c4..791faa24cf 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2885,6 +2885,9 @@ COCCI_SOURCES = $(filter-out $(THIRD_PARTY_SOURCES),$(FOUND_C_SOURCES))\n \tfi\n coccicheck: $(addsuffix .patch,$(filter-out %.pending.cocci,$(wildcard contrib/coccinelle/*.cocci)))\n \n+externcheck: $(filter-out $(THIRD_PARTY_SOURCES),$(filter %.c %.h,$(shell $(FIND_SOURCE_FILES))))\n+\tsed -i 's/^\\(\\s*\\)extern \\([^(]*([^*]\\)/\\1\\2/' $^\n+\n # See contrib/coccinelle/README\n coccicheck-pending: $(addsuffix .patch,$(wildcard contrib/coccinelle/*.pending.cocci))\n \n-- \n2.29.0.rc0.261.g7178c9af9c\n\n"},{"id":"407137","messageId":"nycvar.QRO.7.76.6.2010081125390.50@tvgsbejvaqbjf.bet","threadId":"54362","inReplyTo":"1c2ee1c40ccde18a008262aeaf23a17da2064eae.1602142232.git.liu.denton@gmail.com","subject":"Re: [RFC PATCH] Makefile: create externcheck target","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-10-08T09:32:03Z","receivedAt":"2020-10-08T09:32:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Denton,\n\nOn Thu, 8 Oct 2020, Denton Liu wrote:\n\n> In b199d7147a (*.[ch]: remove extern from function declarations using\n> sed, 2019-04-29), we used sed to remove extern from function\n> definitions. In order to help find and remove future instances of this,\n> teach Makefile the `externcheck` target which runs the sed script\n> included in that commit on all applicable source files.\n>\n> Signed-off-by: Denton Liu <liu.denton@gmail.com>\n> ---\n>\n> Notes:\n>     I run this target periodically to ensure that no new instances of extern\n>     function definitions are introduced. Is this something that we want to\n>     consider adding for real?\n\nSure, and then hook it up in `ci/run-static-analysis.sh`.\n\nBut maybe add it _after_ coccicheck-pending, and add it to the `.PHONY`\nline that's just outside of this patch's context lines?\n\nCiao,\nDscho\n\nP.S.: I am not really certain that the `\\s` is portable, I do not see any\nmention of it in\nhttps://pubs.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap09.html#tag_09_03\nand would therefore expect BSD sed not to handle this correctly. In any\ncase, the `-i` is _not_ portable, as BSD sed takes a mandatory argument\n(see\nhttps://stackoverflow.com/questions/5694228/sed-in-place-flag-that-works-both-on-mac-bsd-and-linux\nfor more details).\n\n>\n>  Makefile | 3 +++\n>  1 file changed, 3 insertions(+)\n>\n> diff --git a/Makefile b/Makefile\n> index 5311b1d2c4..791faa24cf 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2885,6 +2885,9 @@ COCCI_SOURCES = $(filter-out $(THIRD_PARTY_SOURCES),$(FOUND_C_SOURCES))\n>  \tfi\n>  coccicheck: $(addsuffix .patch,$(filter-out %.pending.cocci,$(wildcard contrib/coccinelle/*.cocci)))\n>\n> +externcheck: $(filter-out $(THIRD_PARTY_SOURCES),$(filter %.c %.h,$(shell $(FIND_SOURCE_FILES))))\n> +\tsed -i 's/^\\(\\s*\\)extern \\([^(]*([^*]\\)/\\1\\2/' $^\n> +\n>  # See contrib/coccinelle/README\n>  coccicheck-pending: $(addsuffix .patch,$(wildcard contrib/coccinelle/*.pending.cocci))\n>\n> --\n> 2.29.0.rc0.261.g7178c9af9c\n>\n>\n"},{"id":"407139","messageId":"20201008095233.GA2611970@generichostname","threadId":"54362","inReplyTo":"nycvar.QRO.7.76.6.2010081125390.50@tvgsbejvaqbjf.bet","subject":"Re: [RFC PATCH] Makefile: create externcheck target","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-10-08T09:52:33Z","receivedAt":"2020-10-08T09:52:39Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Dscho,\n\nOn Thu, Oct 08, 2020 at 11:32:03AM +0200, Johannes Schindelin wrote:\n> P.S.: I am not really certain that the `\\s` is portable, I do not see any\n> mention of it in\n> https://pubs.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap09.html#tag_09_03\n> and would therefore expect BSD sed not to handle this correctly.\n\nI've run a varation of this on MacOS's sed before and it works.\nUnfortunately, I don't have access to a MacOS machine anymore so I can't\n100% confirm it.\n\n> In any\n> case, the `-i` is _not_ portable, as BSD sed takes a mandatory argument\n> (see\n> https://stackoverflow.com/questions/5694228/sed-in-place-flag-that-works-both-on-mac-bsd-and-linux\n> for more details).\n\nThis was the main reason why I sent it out as an RFC (although I forgot\nto mention it in the commit message, whoops). Perhaps we could write\nthis as\n\n\texterncheck: $(filter-out $(THIRD_PARTY_SOURCES),$(filter %.c %.h,$(shell $(FIND_SOURCE_FILES))))\n\t\tsed -i.bak 's/^\\(\\s*\\)extern \\([^(]*([^*]\\)/\\1\\2/' $^\n\t\t$(RM) $(addsuffix .bak,$^)\n\ninstead? I think that since this is a developer target, we can probably\ncall this portable enough.\n\nThanks,\nDenton\n"},{"id":"407151","messageId":"nycvar.QRO.7.76.6.2010081625450.50@tvgsbejvaqbjf.bet","threadId":"54362","inReplyTo":"20201008095233.GA2611970@generichostname","subject":"Re: [RFC PATCH] Makefile: create externcheck target","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-10-08T14:26:01Z","receivedAt":"2020-10-08T14:26:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Denton,\n\nOn Thu, 8 Oct 2020, Denton Liu wrote:\n\n> On Thu, Oct 08, 2020 at 11:32:03AM +0200, Johannes Schindelin wrote:\n> > P.S.: I am not really certain that the `\\s` is portable, I do not see any\n> > mention of it in\n> > https://pubs.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap09.html#tag_09_03\n> > and would therefore expect BSD sed not to handle this correctly.\n>\n> I've run a varation of this on MacOS's sed before and it works.\n> Unfortunately, I don't have access to a MacOS machine anymore so I can't\n> 100% confirm it.\n>\n> > In any\n> > case, the `-i` is _not_ portable, as BSD sed takes a mandatory argument\n> > (see\n> > https://stackoverflow.com/questions/5694228/sed-in-place-flag-that-works-both-on-mac-bsd-and-linux\n> > for more details).\n>\n> This was the main reason why I sent it out as an RFC (although I forgot\n> to mention it in the commit message, whoops). Perhaps we could write\n> this as\n>\n> \texterncheck: $(filter-out $(THIRD_PARTY_SOURCES),$(filter %.c %.h,$(shell $(FIND_SOURCE_FILES))))\n> \t\tsed -i.bak 's/^\\(\\s*\\)extern \\([^(]*([^*]\\)/\\1\\2/' $^\n> \t\t$(RM) $(addsuffix .bak,$^)\n>\n> instead? I think that since this is a developer target, we can probably\n> call this portable enough.\n\nYep, I agree.\n\nThanks,\nDscho\n"},{"id":"407193","messageId":"xmqqy2kgungq.fsf@gitster.c.googlers.com","threadId":"54362","inReplyTo":"1c2ee1c40ccde18a008262aeaf23a17da2064eae.1602142232.git.liu.denton@gmail.com","subject":"Re: [RFC PATCH] Makefile: create externcheck target","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-09T02:45:09Z","receivedAt":"2020-10-09T02:45:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu <liu.denton@gmail.com> writes:\n\n> +externcheck: $(filter-out $(THIRD_PARTY_SOURCES),$(filter %.c %.h,$(shell $(FIND_SOURCE_FILES))))\n> +\tsed -i 's/^\\(\\s*\\)extern \\([^(]*([^*]\\)/\\1\\2/' $^\n\nI am not enthused by this particular patch for a few reasons, and I\nam moderately negative on the whole idea.\n\n - We don't aim to support \"only GNU and some BSD\"; let's not do\n   \"-i\" which as far as I know is used only in config.mak.uname for\n   vcxproj target (which is OK as we know that is run only on a very\n   narrow target, but probably is a bad idea as it would be another\n   source of copy-and-paste for those who do not even think why it\n   is acceptable there but not in all other places).\n\n - Same for \\s.  If it is easy enough to write [        ]*, why\n   risk breaking for somebody you don't even know?\n\n - The initial [^*] may be an attempt to avoid triggering on a\n   global pointer-to-function, but doesn't it also make the pattern\n   fail to trigger on a global function whose return type is a\n   pointer-to-function?\n\n - If this is a \"check\" target, we shouldn't apply a wholesale\n   transformation that is potentially buggy to user's files.  Using\n   \"grep\" to just point out the places where your opinion differ\n   from user's (and to fail the \"make foocheck\" operation) would be\n   more appropriate.\n\nQuite honestly, I suspect that the \"push\" that b199d714 (*.[ch]:\nremove extern from function declarations using sed, 2019-04-29)\ntalks about was misguided in the first place.\n\nSure, we can write these external function declarations without\n'extern' in front, because the language allows it and without\n'static' in front, it by default is 'extern'.  It however does\nnot automatically mean we _should_ drop 'extern'.\n\nSure, for function decls, it may not make a difference to have or\nnot have \"extern\" in front, but for decls of data (including\npointers to functions), it makes a whole lot of difference.  Not\nstandardising to the rule \"our external declarations always are\nmarked with leading 'extern', regardless of the type of the\nidentifier being declared\" forces us to spend our brain cycles to\nthink if we should or should not write 'extern' in front.  And is\nthat a good thing to spend our brain cycles on, or just waste of our\neffort?  I am moderately in favor of saying that it is a waste.\n\nIn addition, seeing these 'extern ' in header files will train our\neyes to spot the same in the source files more easily.  External\ndecls in the source (as opposed to inclusion of a header that does\nthe decls) can happen but they ought to be exceptions and it is good\nto make them stand out.\n"}]}