{"thread":{"id":"49898","subject":"[PATCH] config.mak.dev: enable -Wpedantic in clang","startedAt":"2018-11-27T10:06:34Z","lastAt":"2018-11-29T05:13:28Z","messageCount":5,"participants":["Carlo Marcelo Arenas Belón","Eric Sunshine","Carlo Arenas","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"364123","messageId":"20181127100557.53891-1-carenas@gmail.com","threadId":"49898","inReplyTo":null,"subject":"[PATCH] config.mak.dev: enable -Wpedantic in clang","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2018-11-27T10:05:57Z","receivedAt":"2018-11-27T10:06:34Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"DEVOPTS=pedantic adds -pedantic to the compiler flags, but misses on some\ndiagnostics when using clang, and that are only enabled with -Wpedantic\n\n46c0eb5843 (\"files-backend.c: fix build error on Solaris\", 2018-11-25)\nfixes an issue that was visible also with gcc but not clang so correct\nthat with the hope that in the future CI could be used for early detection\nof similar issues\n\n-Wpedantic is only enabled for clang 10 or higher (only available in macOS\nwith latest Xcode) but this restriction should be relaxed further as more\nenvironments are tested\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n config.mak.dev | 11 +++++++----\n 1 file changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/config.mak.dev b/config.mak.dev\nindex bbeeff44fe..ad25beacd8 100644\n--- a/config.mak.dev\n+++ b/config.mak.dev\n@@ -1,8 +1,15 @@\n+ifndef COMPILER_FEATURES\n+COMPILER_FEATURES := $(shell ./detect-compiler $(CC))\n+endif\n+\n ifeq ($(filter no-error,$(DEVOPTS)),)\n CFLAGS += -Werror\n endif\n ifneq ($(filter pedantic,$(DEVOPTS)),)\n CFLAGS += -pedantic\n+ifneq ($(filter clang10,$(COMPILER_FEATURES)),)\n+CFLAGS += -Wpedantic\n+endif\n # don't warn for each N_ use\n CFLAGS += -DUSE_PARENS_AROUND_GETTEXT_N=0\n endif\n@@ -16,10 +23,6 @@ CFLAGS += -Wstrict-prototypes\n CFLAGS += -Wunused\n CFLAGS += -Wvla\n \n-ifndef COMPILER_FEATURES\n-COMPILER_FEATURES := $(shell ./detect-compiler $(CC))\n-endif\n-\n ifneq ($(filter clang4,$(COMPILER_FEATURES)),)\n CFLAGS += -Wtautological-constant-out-of-range-compare\n endif\n-- \n2.20.0.rc1\n\n"},{"id":"364124","messageId":"CAPig+cT1ruGsd-7yXV3Oxs2_9zOh-d4HgZiQ-OVZzHc1buo4FQ@mail.gmail.com","threadId":"49898","inReplyTo":"20181127100557.53891-1-carenas@gmail.com","subject":"Re: [PATCH] config.mak.dev: enable -Wpedantic in clang","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-11-27T10:53:43Z","receivedAt":"2018-11-27T10:53:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Nov 27, 2018 at 5:06 AM Carlo Marcelo Arenas Belón\n wrote:\n> [...]\n> -Wpedantic is only enabled for clang 10 or higher (only available in macOS\n> with latest Xcode) but this restriction should be relaxed further as more\n> environments are tested\n\nWe know from [1] that the clang version number \"10\" is an Apple\nfiction/invention. Perhaps this commit message can be worded a bit\nmore carefully to avoid confusing readers who are not clued in to that\nfact.\n\n[1]: https://public-inbox.org/git/CAPig+cRQXQ_DowS2Dsc1x3TAGJjnWig7P4eYS4kQ+C2piAdSWA@mail.gmail.com/\n\n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> +ifneq ($(filter clang10,$(COMPILER_FEATURES)),)\n> +CFLAGS += -Wpedantic\n> +endif\n\nShould this condition be tightened to match only for OSX since there\nis no such clang version number in the rest world outside of Apple?\n"},{"id":"364136","messageId":"CAPUEsphe5E4ZrLy=NmuAHhdE4jOjYhAstOzHRNtBcuwrhvwbvg@mail.gmail.com","threadId":"49898","inReplyTo":"CAPig+cT1ruGsd-7yXV3Oxs2_9zOh-d4HgZiQ-OVZzHc1buo4FQ@mail.gmail.com","subject":"Re: [PATCH] config.mak.dev: enable -Wpedantic in clang","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2018-11-27T15:48:19Z","receivedAt":"2018-11-27T15:48:35Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Nov 27, 2018 at 2:53 AM Eric Sunshine wrote:\n> On Tue, Nov 27, 2018 at 5:06 AM Carlo Marcelo Arenas Belón\n> > +ifneq ($(filter clang10,$(COMPILER_FEATURES)),)\n> > +CFLAGS += -Wpedantic\n> > +endif\n>\n> Should this condition be tightened to match only for OSX since there\n> is no such clang version number in the rest world outside of Apple?\n\ninstead, I changed it to clang4 and tested it in a FreeBSD 11 box I\nwas able to get a hold off, would that work better?\nwill update patch accordingly then\n\nCarlo\n"},{"id":"364148","messageId":"CAPig+cQXkNo_Sztf3y5ciZ3jjj=XrJ+ESCxEsU_RKxb_0x+STw@mail.gmail.com","threadId":"49898","inReplyTo":"CAPUEsphe5E4ZrLy=NmuAHhdE4jOjYhAstOzHRNtBcuwrhvwbvg@mail.gmail.com","subject":"Re: [PATCH] config.mak.dev: enable -Wpedantic in clang","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-11-27T19:03:43Z","receivedAt":"2018-11-27T19:03:56Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Nov 27, 2018 at 10:48 AM Carlo Arenas wrote:\n> On Tue, Nov 27, 2018 at 2:53 AM Eric Sunshine wrote:\n> > On Tue, Nov 27, 2018 at 5:06 AM Carlo Marcelo Arenas Belón\n> > > +ifneq ($(filter clang10,$(COMPILER_FEATURES)),)\n> > > +CFLAGS += -Wpedantic\n> > > +endif\n> >\n> > Should this condition be tightened to match only for OSX since there\n> > is no such clang version number in the rest world outside of Apple?\n>\n> instead, I changed it to clang4 and tested it in a FreeBSD 11 box I\n> was able to get a hold off, would that work better?\n> will update patch accordingly then\n\nPlaying Devi's Advocate, what if Apple's clang \"8\" was, in reality,\nreal-world clang 3? Then this condition would incorrectly enable the\ncompiler option on Apple for a (real) clang version below 4. For this\nreason, it seems we shouldn't be trusting only the clang version\nnumber when dealing with Apple.\n\n(I suspect that it won't make a difference in actual practice, so it\nmay be reasonable to punt on this issue until/if someone complains.)\n"},{"id":"364285","messageId":"xmqqva4gpits.fsf@gitster-ct.c.googlers.com","threadId":"49898","inReplyTo":"CAPig+cQXkNo_Sztf3y5ciZ3jjj=XrJ+ESCxEsU_RKxb_0x+STw@mail.gmail.com","subject":"Re: [PATCH] config.mak.dev: enable -Wpedantic in clang","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-29T05:13:19Z","receivedAt":"2018-11-29T05:13:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> Playing Devi's Advocate, what if Apple's clang \"8\" was, in reality,\n> real-world clang 3? Then this condition would incorrectly enable the\n> compiler option on Apple for a (real) clang version below 4. For this\n> reason, it seems we shouldn't be trusting only the clang version\n> number when dealing with Apple.\n>\n> (I suspect that it won't make a difference in actual practice, so it\n> may be reasonable to punt on this issue until/if someone complains.)\n\nWhy do we care which true version of clang is being used here in the\nfirst place? Is it because some version of clang (take -Wpedantic\nbut misbehave | barf when -Wpedantic is given) while some others are\nOK?\n\nIf the only problem is that some version of clang barf when the\noption is given, perhaps we can do a trial-compile of helloworld.c\nor something, or is that something we are trying to avoid in the\nfirst place?\n\nIt appears to me that ./detect-compiler tool (by the way, perhaps we\nshould start moving these build-helper-tools sitting at the top level\ndown to ./build-helpers/ subdirectory or something so that we can focus\non the source code when running \"ls\" at the top level of the hierarchy)\ncan become more intelligent to help things like this.\n"}]}