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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri created this revision.
lebedev.ri added reviewers: rsmith, rtrieu, aaron.ballman.
lebedev.ri added a project: clang.

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 adds `warn_extra_semi` (under `-Wextra-semi`), which allows to diagnose
that extra semi even if it is not invalid, and the `-Wc++98-compat-pedantic` is disabled.

This does not answer the questionability of that new warning.
It is **disabled by default**.
This simply allows to diagnose it.

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

Thoughts?


Repository:
  rC Clang

https://reviews.llvm.org/D43162

Files:
  include/clang/Basic/DiagnosticParseKinds.td
  lib/Parse/Parser.cpp
  test/Parser/cxx-extra-semi.cpp
  test/SemaCXX/extra-semi.cpp


Index: test/SemaCXX/extra-semi.cpp
===================================================================
--- /dev/null
+++ test/SemaCXX/extra-semi.cpp
@@ -0,0 +1,26 @@
+// 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 %s
+
+// Last two RUN lines check that -Wextra-semi, which was enabled by
+// -Weverything will still be issued even though -Wc++98-compat contains
+// diag::warn_cxx98_compat_top_level_semi, which would be silenced by -Wno-.
+
+void F();
+
+void F() {}
+; // expected-warning {{extra ';' outside of a function}}
+
+namespace ns {
+class C {
+  void F() const;
+};
+}
+; // expected-warning {{extra ';' outside of a function}}
+
+void ns::C::F() const {}
+; // expected-warning {{extra ';' outside of a function}}
Index: test/Parser/cxx-extra-semi.cpp
===================================================================
--- test/Parser/cxx-extra-semi.cpp
+++ test/Parser/cxx-extra-semi.cpp
@@ -38,4 +38,7 @@
 #if __cplusplus < 201103L
 // expected-warning at -3{{extra ';' outside of a function is a C++11 extension}}
 // expected-warning at -3{{extra ';' outside of a function is a C++11 extension}}
+#elif !defined(PEDANTIC)
+// expected-warning at -6{{extra ';' outside of a function}}
+// expected-warning at -6{{extra ';' outside of a function}}
 #endif
Index: lib/Parse/Parser.cpp
===================================================================
--- lib/Parse/Parser.cpp
+++ lib/Parse/Parser.cpp
@@ -191,12 +191,20 @@
   // C++11 allows extra semicolons at namespace scope, but not in any of the
   // other contexts.
   if (Kind == OutsideFunction && getLangOpts().CPlusPlus) {
+    unsigned DiagID;
+
     if (getLangOpts().CPlusPlus11)
-      Diag(StartLoc, diag::warn_cxx98_compat_top_level_semi)
-          << FixItHint::CreateRemoval(SourceRange(StartLoc, EndLoc));
+      DiagID = diag::warn_cxx98_compat_top_level_semi;
     else
-      Diag(StartLoc, diag::ext_extra_semi_cxx11)
-          << FixItHint::CreateRemoval(SourceRange(StartLoc, EndLoc));
+      DiagID = diag::ext_extra_semi_cxx11;
+
+    // Prefer the pedantic compatibility warning over the generic,
+    // non-extension, user-requested "extra semi" warning.
+    if (Diags.isIgnored(DiagID, EndLoc))
+      DiagID = diag::warn_extra_semi;
+
+    Diag(StartLoc, DiagID)
+        << FixItHint::CreateRemoval(SourceRange(StartLoc, EndLoc));
     return;
   }
 
Index: include/clang/Basic/DiagnosticParseKinds.td
===================================================================
--- include/clang/Basic/DiagnosticParseKinds.td
+++ include/clang/Basic/DiagnosticParseKinds.td
@@ -47,6 +47,9 @@
   "inside instance variable list|"
   "after member function definition}0">,
   InGroup<ExtraSemi>;
+def warn_extra_semi : Warning<
+  "extra ';' outside of a function">,
+  InGroup<ExtraSemi>, DefaultIgnore;
 def ext_extra_semi_cxx11 : Extension<
   "extra ';' outside of a function is a C++11 extension">,
   InGroup<CXX11ExtraSemi>;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D43162.133761.patch
Type: text/x-patch
Size: 3271 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180210/4b80b732/attachment-0001.bin>