[Parser] (C++) Make -Wextra-semi slightly more useful

[lebedev.ri]

Summary

Let's suppose the -Weverything is passed.

Given code like

void F() {}
;

If the code is compiled with -std=c++03, it would diagnose that extra sema:

<source>:2:1: warning: extra ';' outside of a function is a C++11 extension [-Wc++11-extra-semi]
;
^~

If the code is compiled with -std=c++11, it also would diagnose that extra sema:

<source>:2:1: warning: extra ';' outside of a function is incompatible with C++98 [-Wc++98-compat-pedantic]
;
^~

But, let's suppose the C++11 or higher is used, and the used does not care
about -Wc++98-compat-pedantic, so he disables that diagnostic.
And that silences the complaint about extra ; too.
And there is no way to re-enable that particular diagnostic, passing -Wextra-semi does nothing...

Now, there is also a related no newline at end of file diagnostic, which is also emitted by -Wc++98-compat-pedantic

<source>:2:2: warning: C++98 requires newline at end of file [-Wc++98-compat-pedantic]
;
 ^

But unlike the previous case, if -Wno-c++98-compat-pedantic is passed, that diagnostic stays displayed:

<source>:2:2: warning: no newline at end of file [-Wnewline-eof]
;
 ^

This diff refactors the code so -Wc++98-compat-extra-semi can be re-enabled, after the -Wc++98-compat-pedantic was disabled.
This seems ugly, but there does not seem to be any saner way.

Testing: $ ninja check-clang-sema check-clang-semacxx check-clang-lexer check-clang-parser

[lebedev.ri]

Ping.

[lebedev.ri]

After a brief disscussion with @aaron.ballman in IRC,
the question still stands whether this is really the correct approach,
or is there some nicer (TableGen-based?) approach.

Mainly waiting for @rsmith's opinion.

[aaron.ballman]

To be clear -- my concerns were mostly that it seems messy for this sort of thing to require three different diagnostic entries and somewhat convoluted logic to select them -- if we can find a way to generalize this, that'd be better.

[lebedev.ri]

Ping.

[rsmith]

This is the wrong way to deal with this. The only thing that should ever be controlled by -W flags is whether the warnings in that group appear, not whether warnings in other groups appear. The principle is that -W flags should behave "as if" they filter the diagnostic output of Clang. (We actually depend on this in the implicit modules implementation, to allow modules to be reused in compilation modes where a different set of warning flags is enabled.) isIgnored exists only to allow the computations leading to the emission of a diagnostic to be skipped if the diagnostic will not actually be emitted.

The right thing to do is to add a new warning group for the warn_cxx98_compat_top_level_semi warning, and make that a subgroup of the -Wextra-semi group. That way -Wno-extra-semi can be used to turn off both the C++98 extension warning and the C++11 compatibility warning. Take a look at the *BinaryLiteral warning groups for an example of how to do this. (That case is a little more complex because there's also a warning group for the corresponding case in C.)

[lebedev.ri]

Thank you for the feedback!

In D43162#1022427, @rsmith wrote:

This is the wrong way to deal with this. The only thing that should ever be controlled by -W flags is whether the warnings in that group appear, not whether warnings in other groups appear. The principle is that -W flags should behave "as if" they filter the diagnostic output of Clang. (We actually depend on this in the implicit modules implementation, to allow modules to be reused in compilation modes where a different set of warning flags is enabled.) isIgnored exists only to allow the computations leading to the emission of a diagnostic to be skipped if the diagnostic will not actually be emitted.

Aha, i see. To be noted, this is how it is currently already done for warn_cxx98_compat_no_newline_eof/warn_no_newline_eof
https://github.com/llvm-mirror/clang/blob/f828172bcfd7d6d10497c645c3cc5eee321cd669/lib/Lex/Lexer.cpp#L2684-L2695
https://github.com/llvm-mirror/clang/blob/f828172bcfd7d6d10497c645c3cc5eee321cd669/include/clang/Basic/DiagnosticLexKinds.td#L58-L63
So i'm guessing that^ should be fixed too?

The right thing to do is to add a new warning group for the warn_cxx98_compat_top_level_semi warning, and make that a subgroup of the -Wextra-semi group. That way -Wno-extra-semi can be used to turn off both the C++98 extension warning and the C++11 compatibility warning. Take a look at the *BinaryLiteral warning groups for an example of how to do this. (That case is a little more complex because there's also a warning group for the corresponding case in C.)

I'll look into it, but *right now* this sounds like it won't do what i'm trying to do here. Hopefully i'm just missing the point :)

[lebedev.ri]

Reworked stuff via new CXX98CompatExtraSemi diag group.

As expected, passing -Wno-c++98-compat-pedantic disables the extra-semi diag, which exactly the opposite from what i'm trying to do here...
But now that the diag is moved to the new diag group, it can be re-enabled, which is at least something. But this looks kinda ugly.
Is this how it should be?

[lebedev.ri]

If i'm reading git blame correctly, the -Wnewline-eof diag, which i used as a base to model the previous version of this diff on,
was added in rL189110 / https://github.com/llvm-mirror/clang/commit/7865b8e4324378e06f59adb4d60bec26a7d3d584 by @jordan_rose.
Since the @rsmith's review note suggests the approach is incorrect, i've added the author of that commit as a subscriber, just in case.

[jordan_rose]

It's been a long time since that commit, but I think the difference is that -pedantic / Extension-type diagnostics are magic and -Wc++98-compat-pedantic is not. I like Richard's way better and if it could be applied to -Wnewline-eof later as well then that sounds great.

[lebedev.ri]

Ping

[lebedev.ri]

Let's move weekly ping away from friday :)
Ping.

[aaron.ballman]

LGTM aside from a testing nit.

test/SemaCXX/extra-semi.cpp

14

Can you use the full diagnostic text for this first appearance in the test?

[lebedev.ri]

test/SemaCXX/extra-semi.cpp

14

Will require preprocessor, because these are two different messages depending of -std=
Will do.

include/clang/Basic/DiagnosticGroups.td

def Documentation : DiagGroup<"documentation",
                                DocumentationDeprecatedSync]>;
 
 def EmptyBody : DiagGroup<"empty-body">;
 def Exceptions : DiagGroup<"exceptions">;
 
 def GNUEmptyInitializer : DiagGroup<"gnu-empty-initializer">;
 def GNUEmptyStruct : DiagGroup<"gnu-empty-struct">;
 def ExtraTokens : DiagGroup<"extra-tokens">;
+def CXX98CompatExtraSemi : DiagGroup<"c++98-compat-extra-semi">;
 def CXX11ExtraSemi : DiagGroup<"c++11-extra-semi">;
-def ExtraSemi : DiagGroup<"extra-semi", [CXX11ExtraSemi]>;
+def ExtraSemi : DiagGroup<"extra-semi", [CXX98CompatExtraSemi,
+                                         CXX11ExtraSemi]>;
 
 def GNUFlexibleArrayInitializer : DiagGroup<"gnu-flexible-array-initializer">;
 def GNUFlexibleArrayUnionMember : DiagGroup<"gnu-flexible-array-union-member">;
 def GNUFoldingConstant : DiagGroup<"gnu-folding-constant">;
 def FormatExtraArgs : DiagGroup<"format-extra-args">;
 def FormatZeroLength : DiagGroup<"format-zero-length">;
 
 def InvalidIOSDeploymentTarget : DiagGroup<"invalid-ios-deployment-target">;
def CXX98Compat : DiagGroup<"c++98-compat",
                              CXX98CompatUnnamedTypeTemplateArgs,
                              CXXPre14Compat,
                              CXXPre17Compat,
                              CXXPre2aCompat]>;
 // Warnings for C++11 features which are Extensions in C++98 mode.
 def CXX98CompatPedantic : DiagGroup<"c++98-compat-pedantic",
                                     [CXX98Compat,
                                      CXX98CompatBindToTemporaryCopy,
+                                     CXX98CompatExtraSemi,
                                      CXXPre14CompatPedantic,
                                      CXXPre17CompatPedantic,
                                      CXXPre2aCompatPedantic]>;
 
 def CXX11Narrowing : DiagGroup<"c++11-narrowing">;
 
 def CXX11WarnOverrideDestructor :
   DiagGroup<"inconsistent-missing-destructor-override">;

include/clang/Basic/DiagnosticParseKinds.td

 
 let CategoryName = "Parse Issue" in {
 
 def ext_empty_translation_unit : Extension<
   "ISO C requires a translation unit to contain at least one declaration">,
   InGroup<DiagGroup<"empty-translation-unit">>;
 def warn_cxx98_compat_top_level_semi : Warning<
   "extra ';' outside of a function is incompatible with C++98">,
-  InGroup<CXX98CompatPedantic>, DefaultIgnore;
+  InGroup<CXX98CompatExtraSemi>, DefaultIgnore;
 def ext_extra_semi : Extension<
   "extra ';' %select{"
   "outside of a function|"
   "inside a %1|"
   "inside instance variable list|"
   "after member function definition}0">,
   InGroup<ExtraSemi>;
 def ext_extra_semi_cxx11 : Extension<

test/Parser/cxx-extra-semi.cpp

union B {
   int a2;; // expected-warning{{extra ';' inside a union}}
 };
 
 ;
 ; ;;
 #if __cplusplus < 201103L
 // expected-warning@-3{{extra ';' outside of a function is a C++11 extension}}
 // expected-warning@-3{{extra ';' outside of a function is a C++11 extension}}
+#elif !defined(PEDANTIC)
+// expected-warning@-6{{extra ';' outside of a function is incompatible with C++98}}
+// expected-warning@-6{{extra ';' outside of a function is incompatible with C++98}}
 #endif

test/SemaCXX/extra-semi.cpp

+// RUN: %clang_cc1 -verify -std=c++98 -Wextra-semi %s
+// RUN: %clang_cc1 -verify -std=c++03 -Wextra-semi %s
+// RUN: %clang_cc1 -verify -std=c++11 -Wextra-semi %s
+// RUN: %clang_cc1 -verify -std=c++17 -Wextra-semi %s
+// RUN: %clang_cc1 -verify -std=c++2a -Wextra-semi %s
+// RUN: %clang_cc1 -verify -Weverything -Wno-c++98-compat %s
+// RUN: %clang_cc1 -verify -Weverything -Wno-c++98-compat-pedantic -Wc++98-compat-extra-semi %s
+
+// Last RUN line checks that c++98-compat-extra-semi can still be re-enabled.
+
+void F();
+
+void F(){}
+; // expected-warning {{extra ';' outside of a function is}}
+
+namespace ns {
+class C {
+  void F() const;
+};
+}
+; // expected-warning {{extra ';' outside of a function is}}
+
+void ns::C::F() const {}
+; // expected-warning {{extra ';' outside of a function is}}