lebedev.ri created this revision.
lebedev.ri added reviewers: rsmith, aaron.ballman, efriedma.
clang has `-Wextra-semi` (https://reviews.llvm.org/D43162), which is not
dictated by the currently selected standard.
While that is great, there is at least one more source of need-less semis -
'null statements'.
Sometimes, they are needed:
for(int x = 0; continueToDoWork(x); x++)
; // Ugly code, but the semi is needed here.
But sometimes they are just there for no reason:
switch(X) {
case 0:
return -2345;
case 5:
return 0;
default:
return 42;
}; // <- oops
;;;;;;;;;;; <- OOOOPS, still not diagnosed. Clearly this is junk.
As usual, things may or may not go sideways in the presence of macros.
While evaluating this diag on my codebase of interest, it was unsurprisingly
discovered that Google Test macros are *very* prone to this.
And it seems many issues are deep within the GTest itself, not
in the snippets passed from the codebase that uses GTest.
So after some thought, i decided not do issue a diagnostic if the semi
is within *any* macro, be it either from the normal header, or system header.
Fixes PR39111 <https://bugs.llvm.org/show_bug.cgi?id=39111>
Repository:
rC Clang
https://reviews.llvm.org/D52695
Files:
docs/ReleaseNotes.rst
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticParseKinds.td
lib/Parse/ParseStmt.cpp
test/Parser/extra-semi-resulting-in-nullstmt.cpp
Index: test/Parser/extra-semi-resulting-in-nullstmt.cpp
===================================================================
--- /dev/null
+++ test/Parser/extra-semi-resulting-in-nullstmt.cpp
@@ -0,0 +1,62 @@
+// RUN: %clang_cc1 -fsyntax-only -Wextra-semi-pedantic -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wextra-semi-pedantic -std=c++17 -verify %s
+// RUN: cp %s %t
+// RUN: %clang_cc1 -x c++ -Wextra-semi-pedantic -fixit %t
+// RUN: %clang_cc1 -x c++ -Wextra-semi-pedantic -Werror %t
+
+#define GOODMACRO(varname) int varname
+#define BETTERMACRO(varname) GOODMACRO(varname);
+#define NULLMACRO(varname)
+
+void test() {
+ ; // expected-warning {{';' with no preceding expression is a null statement}}
+ ; // expected-warning {{';' with no preceding expression is a null statement}}
+
+ // clang-format: off
+ ;; // expected-warning 2 {{';' with no preceding expression is a null statement}}
+ // clang-format: on
+
+ {}; // expected-warning {{';' with no preceding expression is a null statement}}
+
+ {
+ ; // expected-warning {{';' with no preceding expression is a null statement}}
+ }
+
+ if (true) {
+ ; // expected-warning {{';' with no preceding expression is a null statement}}
+ }
+
+ GOODMACRO(v0); // OK
+
+ GOODMACRO(v1;) // OK
+
+ BETTERMACRO(v2) // OK
+
+ BETTERMACRO(v3;) // Extra ';', but within macro expansion, so ignored.
+
+ BETTERMACRO(v4); // expected-warning {{';' with no preceding expression is a null statement}}
+
+ BETTERMACRO(v5;); // expected-warning {{';' with no preceding expression is a null statement}}
+
+ NULLMACRO(v6) // OK
+
+ NULLMACRO(v7); // OK. This could be either GOODMACRO() or BETTERMACRO() situation, so we can't know we can drop it.
+
+ if (true)
+ ; // OK
+
+ while (true)
+ ; // OK
+
+ do
+ ; // OK
+ while (true);
+
+ for (;;) // OK
+ ; // OK
+
+#if __cplusplus >= 201703L
+ if (; true) // OK
+ ; // OK
+#endif
+}
Index: lib/Parse/ParseStmt.cpp
===================================================================
--- lib/Parse/ParseStmt.cpp
+++ lib/Parse/ParseStmt.cpp
@@ -233,7 +233,13 @@
return ParseCompoundStatement();
case tok::semi: { // C99 6.8.3p3: expression[opt] ';'
bool HasLeadingEmptyMacro = Tok.hasLeadingEmptyMacro();
- return Actions.ActOnNullStmt(ConsumeToken(), HasLeadingEmptyMacro);
+ SourceLocation SemiLocation = ConsumeToken();
+ if (!HasLeadingEmptyMacro && getCurScope()->isCompoundStmtScope() &&
+ !SemiLocation.isMacroID()) {
+ Diag(SemiLocation, diag::warn_null_statement)
+ << FixItHint::CreateRemoval(SemiLocation);
+ }
+ return Actions.ActOnNullStmt(SemiLocation, HasLeadingEmptyMacro);
}
case tok::kw_if: // C99 6.8.4.1: if-statement
Index: include/clang/Basic/DiagnosticParseKinds.td
===================================================================
--- include/clang/Basic/DiagnosticParseKinds.td
+++ include/clang/Basic/DiagnosticParseKinds.td
@@ -53,6 +53,9 @@
def warn_extra_semi_after_mem_fn_def : Warning<
"extra ';' after member function definition">,
InGroup<ExtraSemi>, DefaultIgnore;
+def warn_null_statement : Warning<
+ "';' with no preceding expression is a null statement">,
+ InGroup<ExtraSemiPedantic>, DefaultIgnore;
def ext_thread_before : Extension<"'__thread' before '%0'">;
def ext_keyword_as_ident : ExtWarn<
Index: include/clang/Basic/DiagnosticGroups.td
===================================================================
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -160,8 +160,10 @@
def ExtraTokens : DiagGroup<"extra-tokens">;
def CXX98CompatExtraSemi : DiagGroup<"c++98-compat-extra-semi">;
def CXX11ExtraSemi : DiagGroup<"c++11-extra-semi">;
+def ExtraSemiPedantic : DiagGroup<"extra-semi-pedantic">;
def ExtraSemi : DiagGroup<"extra-semi", [CXX98CompatExtraSemi,
- CXX11ExtraSemi]>;
+ CXX11ExtraSemi,
+ ExtraSemiPedantic]>;
def GNUFlexibleArrayInitializer : DiagGroup<"gnu-flexible-array-initializer">;
def GNUFlexibleArrayUnionMember : DiagGroup<"gnu-flexible-array-union-member">;
Index: docs/ReleaseNotes.rst
===================================================================
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -50,6 +50,28 @@
Improvements to Clang's diagnostics
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+- ``-Wextra-semi-pedantic`` is a new diagnostic, which is, much like
+ ``-Wextra-semi``, diagnoses extra semicolons. This new diagnostic
+ fires on all *unnecessary* null statements (expression statements without
+ an expression), unless: the semi directly follows a macro that was expanded
+ to nothing; the semi is within the macro itself (both macros from system
+ headers, and normal macros).
+
+ .. code-block:: c++
+
+ #define MACRO(x) int x;
+
+ void test() {
+ ; // <- warning: ';' with no preceding expression is a null statement
+
+ while (true)
+ ; // OK, it is needed.
+
+ MACRO(v0;) // Extra semi, but within macro, so ignored.
+
+ MACRO(v1); // <- warning: ';' with no preceding expression is a null statement
+ }
+
Non-comprehensive list of changes in this release
-------------------------------------------------
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
