[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)
This revision was automatically updated to reflect the committed changes.
Closed by commit rC347339: [clang][Parse] Diagnose useless null statements /
empty init-statements (authored by lebedevri, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D52695?vs=174624=174812#toc
Repository:
rC Clang
https://reviews.llvm.org/D52695
Files:
docs/ReleaseNotes.rst
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticParseKinds.td
include/clang/Parse/Parser.h
lib/Parse/ParseExprCXX.cpp
lib/Parse/ParseStmt.cpp
test/Parser/extra-semi-resulting-in-nullstmt-in-init-statement.cpp
test/Parser/extra-semi-resulting-in-nullstmt.cpp
Index: include/clang/Basic/DiagnosticParseKinds.td
===
--- include/clang/Basic/DiagnosticParseKinds.td
+++ include/clang/Basic/DiagnosticParseKinds.td
@@ -53,6 +53,10 @@
def warn_extra_semi_after_mem_fn_def : Warning<
"extra ';' after member function definition">,
InGroup, DefaultIgnore;
+def warn_null_statement : Warning<
+ "empty expression statement has no effect; "
+ "remove unnecessary ';' to silence this warning">,
+ InGroup, DefaultIgnore;
def ext_thread_before : Extension<"'__thread' before '%0'">;
def ext_keyword_as_ident : ExtWarn<
@@ -554,6 +558,9 @@
def warn_cxx17_compat_for_range_init_stmt : Warning<
"range-based for loop initialization statements are incompatible with "
"C++ standards before C++2a">, DefaultIgnore, InGroup;
+def warn_empty_init_statement : Warning<
+ "empty initialization statement of '%select{if|switch|range-based for}0' "
+ "has no effect">, InGroup, DefaultIgnore;
// C++ derived classes
def err_dup_virtual : Error<"duplicate 'virtual' in base specifier">;
Index: include/clang/Basic/DiagnosticGroups.td
===
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -162,6 +162,8 @@
def ExtraTokens : DiagGroup<"extra-tokens">;
def CXX98CompatExtraSemi : DiagGroup<"c++98-compat-extra-semi">;
def CXX11ExtraSemi : DiagGroup<"c++11-extra-semi">;
+def EmptyInitStatement : DiagGroup<"empty-init-stmt">;
+def ExtraSemiStmt : DiagGroup<"extra-semi-stmt", [EmptyInitStatement]>;
def ExtraSemi : DiagGroup<"extra-semi", [CXX98CompatExtraSemi,
CXX11ExtraSemi]>;
@@ -768,7 +770,8 @@
MissingMethodReturnType,
SignCompare,
UnusedParameter,
-NullPointerArithmetic
+NullPointerArithmetic,
+EmptyInitStatement
]>;
def Most : DiagGroup<"most", [
Index: include/clang/Parse/Parser.h
===
--- include/clang/Parse/Parser.h
+++ include/clang/Parse/Parser.h
@@ -1888,6 +1888,7 @@
StmtResult ParseCompoundStatement(bool isStmtExpr,
unsigned ScopeFlags);
void ParseCompoundStatementLeadingPragmas();
+ bool ConsumeNullStmt(StmtVector );
StmtResult ParseCompoundStatementBody(bool isStmtExpr = false);
bool ParseParenExprOrCondition(StmtResult *InitStmt,
Sema::ConditionResult ,
Index: test/Parser/extra-semi-resulting-in-nullstmt.cpp
===
--- test/Parser/extra-semi-resulting-in-nullstmt.cpp
+++ test/Parser/extra-semi-resulting-in-nullstmt.cpp
@@ -0,0 +1,96 @@
+// RUN: %clang_cc1 -fsyntax-only -Wextra-semi-stmt -verify %s
+// RUN: cp %s %t
+// RUN: %clang_cc1 -x c++ -Wextra-semi-stmt -fixit %t
+// RUN: %clang_cc1 -x c++ -Wextra-semi-stmt -Werror %t
+
+#define GOODMACRO(varname) int varname
+#define BETTERMACRO(varname) GOODMACRO(varname);
+#define NULLMACRO(varname)
+
+enum MyEnum {
+ E1,
+ E2
+};
+
+void test() {
+ ; // expected-warning {{empty expression statement has no effect; remove unnecessary ';' to silence this warning}}
+ ;
+
+ // This removal of extra semi also consumes all the comments.
+ // clang-format: off
+ ;;;
+ // clang-format: on
+
+ // clang-format: off
+ ;NULLMACRO(ZZ);
+ // clang-format: on
+
+ {}; // expected-warning {{empty expression statement has no effect; remove unnecessary ';' to silence this warning}}
+
+ {
+; // expected-warning {{empty expression statement has no effect; remove unnecessary ';' to silence this warning}}
+ }
+
+ if (true) {
+; // expected-warning {{empty expression statement has no effect; remove unnecessary ';' to silence this warning}}
+ }
+
+ GOODMACRO(v0); // OK
+
+ GOODMACRO(v1;) // OK
+
+ BETTERMACRO(v2) // OK
+
+ BETTERMACRO(v3;) // Extra ';', but within macro expansion, so ignored.
+
+ BETTERMACRO(v4); // expected-warning {{empty expression statement has no effect; remove unnecessary ';' to silence this warning}}
+
+ BETTERMACRO(v5;); // expected-warning {{empty expression statement has no effect; remove unnecessary ';' to silence this warning}}
+
+ NULLMACRO(v6) // OK
+
+ NULLMACRO(v7); // OK.
[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)
lebedev.ri added a comment. In https://reviews.llvm.org/D52695#1304225, @aaron.ballman wrote: > Aside from some small nits, LGTM! Thank you for the review! Will address nits and land. Repository: rC Clang https://reviews.llvm.org/D52695 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Aside from some small nits, LGTM! Comment at: docs/ReleaseNotes.rst:53 +- ``-Wextra-semi-stmt`` is a new diagnostic, which is, much like + ``-Wextra-semi``, diagnoses extra semicolons. This new diagnostic aaron.ballman wrote: > , which is, much like `-Wextra-semi`, diagnoses extra semicolons. -> that > diagnoses extra semicolons, much like `-Wextra-semi`. diagnostic, that -> diagnostic that Comment at: docs/ReleaseNotes.rst:93 + of ``if``, ``switch``, ``range-based for``, unless: the semicolon directly + follows a macro that was expanded nothing or if the semicolon is within the + macro itself (both macros from system headers, and normal macros). This expanded nothing -> expanded to nothing Comment at: include/clang/Basic/DiagnosticParseKinds.td:57 +def warn_null_statement : Warning< + " empty expression statement has no effect; " + "remove unnecessary ';' to silence this warning">, Spurious whitespace at the start of the diagnostic text. Repository: rC Clang https://reviews.llvm.org/D52695 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)
lebedev.ri updated this revision to Diff 174624.
lebedev.ri marked 12 inline comments as done.
lebedev.ri added a comment.
Address @aaron.ballman review notes.
Repository:
rC Clang
https://reviews.llvm.org/D52695
Files:
docs/ReleaseNotes.rst
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticParseKinds.td
include/clang/Parse/Parser.h
lib/Parse/ParseExprCXX.cpp
lib/Parse/ParseStmt.cpp
test/Parser/extra-semi-resulting-in-nullstmt-in-init-statement.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,96 @@
+// RUN: %clang_cc1 -fsyntax-only -Wextra-semi-stmt -verify %s
+// RUN: cp %s %t
+// RUN: %clang_cc1 -x c++ -Wextra-semi-stmt -fixit %t
+// RUN: %clang_cc1 -x c++ -Wextra-semi-stmt -Werror %t
+
+#define GOODMACRO(varname) int varname
+#define BETTERMACRO(varname) GOODMACRO(varname);
+#define NULLMACRO(varname)
+
+enum MyEnum {
+ E1,
+ E2
+};
+
+void test() {
+ ; // expected-warning {{empty expression statement has no effect; remove unnecessary ';' to silence this warning}}
+ ;
+
+ // This removal of extra semi also consumes all the comments.
+ // clang-format: off
+ ;;;
+ // clang-format: on
+
+ // clang-format: off
+ ;NULLMACRO(ZZ);
+ // clang-format: on
+
+ {}; // expected-warning {{empty expression statement has no effect; remove unnecessary ';' to silence this warning}}
+
+ {
+; // expected-warning {{empty expression statement has no effect; remove unnecessary ';' to silence this warning}}
+ }
+
+ if (true) {
+; // expected-warning {{empty expression statement has no effect; remove unnecessary ';' to silence this warning}}
+ }
+
+ GOODMACRO(v0); // OK
+
+ GOODMACRO(v1;) // OK
+
+ BETTERMACRO(v2) // OK
+
+ BETTERMACRO(v3;) // Extra ';', but within macro expansion, so ignored.
+
+ BETTERMACRO(v4); // expected-warning {{empty expression statement has no effect; remove unnecessary ';' to silence this warning}}
+
+ BETTERMACRO(v5;); // expected-warning {{empty expression statement has no effect; remove unnecessary ';' to silence this warning}}
+
+ 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
+
+ MyEnum my_enum;
+ switch (my_enum) {
+ case E1:
+// stuff
+break;
+ case E2:; // OK
+ }
+
+ for (;;) {
+for (;;) {
+ goto contin_outer;
+}
+ contin_outer:; // OK
+ }
+}
+
+;
+
+namespace NS {};
+
+void foo(int x) {
+ switch (x) {
+ case 0:
+[[fallthrough]];
+ case 1:
+return;
+ }
+}
+
+[[]];
Index: test/Parser/extra-semi-resulting-in-nullstmt-in-init-statement.cpp
===
--- /dev/null
+++ test/Parser/extra-semi-resulting-in-nullstmt-in-init-statement.cpp
@@ -0,0 +1,64 @@
+// RUN: %clang_cc1 -fsyntax-only -Wextra -std=c++2a -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wextra-semi-stmt -std=c++2a -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wempty-init-stmt -std=c++2a -verify %s
+// RUN: cp %s %t
+// RUN: %clang_cc1 -x c++ -Wempty-init-stmt -std=c++2a -fixit %t
+// RUN: %clang_cc1 -x c++ -Wempty-init-stmt -std=c++2a -Werror %t
+
+struct S {
+ int *begin();
+ int *end();
+};
+
+void naive(int x) {
+ if (; true) // expected-warning {{empty initialization statement of 'if' has no effect}}
+;
+
+ switch (; x) { // expected-warning {{empty initialization statement of 'switch' has no effect}}
+ }
+
+ for (; int y : S()) // expected-warning {{empty initialization statement of 'range-based for' has no effect}}
+;
+
+ for (;;) // OK
+;
+}
+
+#define NULLMACRO
+
+void with_null_macro(int x) {
+ if (NULLMACRO; true)
+;
+
+ switch (NULLMACRO; x) {
+ }
+
+ for (NULLMACRO; int y : S())
+;
+}
+
+#define SEMIMACRO ;
+
+void with_semi_macro(int x) {
+ if (SEMIMACRO true)
+;
+
+ switch (SEMIMACRO x) {
+ }
+
+ for (SEMIMACRO int y : S())
+;
+}
+
+#define PASSTHROUGHMACRO(x) x
+
+void with_passthrough_macro(int x) {
+ if (PASSTHROUGHMACRO(;) true)
+;
+
+ switch (PASSTHROUGHMACRO(;) x) {
+ }
+
+ for (PASSTHROUGHMACRO(;) int y : S())
+;
+}
Index: lib/Parse/ParseStmt.cpp
===
--- lib/Parse/ParseStmt.cpp
+++ lib/Parse/ParseStmt.cpp
@@ -930,6 +930,34 @@
}
+/// Consume any extra semi-colons resulting in null statements,
+/// returning true if any tok::semi were consumed.
+bool Parser::ConsumeNullStmt(StmtVector ) {
+ if (!Tok.is(tok::semi))
+return false;
+
+ SourceLocation StartLoc = Tok.getLocation();
+ SourceLocation EndLoc;
+
+ while (Tok.is(tok::semi) && !Tok.hasLeadingEmptyMacro() &&
+
[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)
lebedev.ri added inline comments.
Comment at: test/Parser/extra-semi-resulting-in-nullstmt.cpp:81
+ }
+}
aaron.ballman wrote:
> There are four additional tests I'd like to see: 1) a semicolon at global
> scope, 2) a semicolon after a namespace declaration (e.g., `namespace foo
> {};`), 3) a `[[fallthrough]];` null statement, 4) a `[[]];` null statement.
None of these warn, added.
Repository:
rC Clang
https://reviews.llvm.org/D52695
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)
aaron.ballman added a comment.
Sorry about the delayed review on this -- I had intended to provide comments
earlier, but somehow this fell off my radar.
Comment at: docs/ReleaseNotes.rst:53
+- ``-Wextra-semi-stmt`` is a new diagnostic, which is, much like
+ ``-Wextra-semi``, diagnoses extra semicolons. This new diagnostic
, which is, much like `-Wextra-semi`, diagnoses extra semicolons. -> that
diagnoses extra semicolons, much like `-Wextra-semi`.
Comment at: docs/ReleaseNotes.rst:55
+ ``-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
fires on all -> diagnoses
Comment at: docs/ReleaseNotes.rst:56
+ 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
semi -> semicolon
Comment at: docs/ReleaseNotes.rst:57
+ 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).
nothing; the semi is -> nothing or if the semicolon
I'd make the parenthetical a stand-alone sentence. Something like: "This
applies to macros defined in system headers as well as user-defined macros."
Comment at: docs/ReleaseNotes.rst:65-68
+ struct S {
+int *begin();
+int *end();
+ };
I'd drop this from the example.
Comment at: docs/ReleaseNotes.rst:91
+
+- ``-Wempty-init-stmt`` is a new diagnostic, which is diagnoses empty
+ init-statements of ``if``, ``switch``, ``range-based for``, unless: the semi
diagnostic, which is diagnoses empty -> diagnostic that diagnoses empty
Comment at: docs/ReleaseNotes.rst:92
+- ``-Wempty-init-stmt`` is a new diagnostic, which is diagnoses empty
+ init-statements of ``if``, ``switch``, ``range-based for``, unless: the semi
+ directly follows a macro that was expanded to nothing; the semi is within the
semi -> semicolon
Comment at: docs/ReleaseNotes.rst:93-94
+ init-statements of ``if``, ``switch``, ``range-based for``, 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).
+ This diagnostic in ``-Wextra-semi-stmt`` group, and is enabled in
``-Wextra``.
nothing; the semi is -> nothing or if the semicolon
I'd make the parenthetical a stand-alone sentence. Something like: "This
applies to macros defined in system headers as well as user-defined macros."
Comment at: docs/ReleaseNotes.rst:95
+ macro itself (both macros from system headers, and normal macros).
+ This diagnostic in ``-Wextra-semi-stmt`` group, and is enabled in
``-Wextra``.
+
diagnostic in -> diagnostic is in the
Drop the comma.
Comment at: include/clang/Basic/DiagnosticParseKinds.td:57-58
+def warn_null_statement : Warning<
+ "statement has no effect, ';' "
+ "with no preceding expression is a null statement">,
+ InGroup, DefaultIgnore;
How about: `empty expression statement has no effect; remove unnecessary ';' to
silence this warning` ?
Comment at: include/clang/Basic/DiagnosticParseKinds.td:557-558
+def warn_empty_init_statement : Warning<
+ "init-statement of '%select{if|switch|range-based for}0' is a "
+ "null statement">, InGroup, DefaultIgnore;
How about: `empty initialization statement of '%select{if|switch|range-based
for}0' has no effect`?
Comment at: test/Parser/extra-semi-resulting-in-nullstmt.cpp:81
+ }
+}
There are four additional tests I'd like to see: 1) a semicolon at global
scope, 2) a semicolon after a namespace declaration (e.g., `namespace foo
{};`), 3) a `[[fallthrough]];` null statement, 4) a `[[]];` null statement.
Repository:
rC Clang
https://reviews.llvm.org/D52695
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)
lebedev.ri added a comment. Ping. Is even the `-Wempty-init-stmt` is not interesting? Repository: rC Clang https://reviews.llvm.org/D52695 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)
lebedev.ri added a comment. ping. I wonder if @aaron.ballman has any opinion on this. Repository: rC Clang https://reviews.llvm.org/D52695 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)
lebedev.ri added a comment. Ping. Repository: rC Clang https://reviews.llvm.org/D52695 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)
lebedev.ri added a comment. ping Repository: rC Clang https://reviews.llvm.org/D52695 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)
lebedev.ri added a comment. Ping. Repository: rC Clang https://reviews.llvm.org/D52695 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)
lebedev.ri added a comment. Ping. Repository: rC Clang https://reviews.llvm.org/D52695 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)
lebedev.ri updated this revision to Diff 168240.
lebedev.ri marked an inline comment as done.
lebedev.ri added a comment.
Thank you for taking a look!
- Reworded the diag as suggested a little
- Do consume consequtive semis as one
- Do still record each one of them in AST. I'm not sure if we want to do
that, but else existing tests break.
Repository:
rC Clang
https://reviews.llvm.org/D52695
Files:
docs/ReleaseNotes.rst
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticParseKinds.td
include/clang/Parse/Parser.h
lib/Parse/ParseExprCXX.cpp
lib/Parse/ParseStmt.cpp
test/Parser/extra-semi-resulting-in-nullstmt-in-init-statement.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,81 @@
+// RUN: %clang_cc1 -fsyntax-only -Wextra-semi-stmt -verify %s
+// RUN: cp %s %t
+// RUN: %clang_cc1 -x c++ -Wextra-semi-stmt -fixit %t
+// RUN: %clang_cc1 -x c++ -Wextra-semi-stmt -Werror %t
+
+#define GOODMACRO(varname) int varname
+#define BETTERMACRO(varname) GOODMACRO(varname);
+#define NULLMACRO(varname)
+
+enum MyEnum {
+ E1,
+ E2
+};
+
+void test() {
+ ; // expected-warning {{statement has no effect, ';' with no preceding expression is a null statement}}
+ ;
+
+ // This removal of extra semi also consumes all the comments.
+ // clang-format: off
+ ;;;
+ // clang-format: on
+
+ // clang-format: off
+ ;NULLMACRO(ZZ);
+ // clang-format: on
+
+ {}; // expected-warning {{statement has no effect, ';' with no preceding expression is a null statement}}
+
+ {
+; // expected-warning {{statement has no effect, ';' with no preceding expression is a null statement}}
+ }
+
+ if (true) {
+; // expected-warning {{statement has no effect, ';' 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 {{statement has no effect, ';' with no preceding expression is a null statement}}
+
+ BETTERMACRO(v5;); // expected-warning {{statement has no effect, ';' 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
+
+ MyEnum my_enum;
+ switch (my_enum) {
+ case E1:
+// stuff
+break;
+ case E2:; // OK
+ }
+
+ for (;;) {
+for (;;) {
+ goto contin_outer;
+}
+ contin_outer:; // OK
+ }
+}
Index: test/Parser/extra-semi-resulting-in-nullstmt-in-init-statement.cpp
===
--- /dev/null
+++ test/Parser/extra-semi-resulting-in-nullstmt-in-init-statement.cpp
@@ -0,0 +1,64 @@
+// RUN: %clang_cc1 -fsyntax-only -Wextra -std=c++2a -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wextra-semi-stmt -std=c++2a -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wempty-init-stmt -std=c++2a -verify %s
+// RUN: cp %s %t
+// RUN: %clang_cc1 -x c++ -Wempty-init-stmt -std=c++2a -fixit %t
+// RUN: %clang_cc1 -x c++ -Wempty-init-stmt -std=c++2a -Werror %t
+
+struct S {
+ int *begin();
+ int *end();
+};
+
+void naive(int x) {
+ if (; true) // expected-warning {{init-statement of 'if' is a null statement}}
+;
+
+ switch (; x) { // expected-warning {{init-statement of 'switch' is a null statement}}
+ }
+
+ for (; int y : S()) // expected-warning {{init-statement of 'range-based for' is a null statement}}
+;
+
+ for (;;) // OK
+;
+}
+
+#define NULLMACRO
+
+void with_null_macro(int x) {
+ if (NULLMACRO; true)
+;
+
+ switch (NULLMACRO; x) {
+ }
+
+ for (NULLMACRO; int y : S())
+;
+}
+
+#define SEMIMACRO ;
+
+void with_semi_macro(int x) {
+ if (SEMIMACRO true)
+;
+
+ switch (SEMIMACRO x) {
+ }
+
+ for (SEMIMACRO int y : S())
+;
+}
+
+#define PASSTHROUGHMACRO(x) x
+
+void with_passthrough_macro(int x) {
+ if (PASSTHROUGHMACRO(;) true)
+;
+
+ switch (PASSTHROUGHMACRO(;) x) {
+ }
+
+ for (PASSTHROUGHMACRO(;) int y : S())
+;
+}
Index: lib/Parse/ParseStmt.cpp
===
--- lib/Parse/ParseStmt.cpp
+++ lib/Parse/ParseStmt.cpp
@@ -930,6 +930,34 @@
}
+/// Consume any extra semi-colons resulting in null statements,
+/// returning true if any tok::semi were consumed.
+bool Parser::ConsumeNullStmt(StmtVector ) {
+ if (!Tok.is(tok::semi))
+return false;
+
+ SourceLocation StartLoc = Tok.getLocation();
+ SourceLocation EndLoc;
+
+ while (Tok.is(tok::semi) && !Tok.hasLeadingEmptyMacro() &&
+ Tok.getLocation().isValid() && !Tok.getLocation().isMacroID())
[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)
rsmith added a comment. I feel indifferent about this warning, but clearly you care about it enough to work on it, and it's a pretty low-cost feature from a maintenance perspective, so I suppose it's fine. Comment at: include/clang/Basic/DiagnosticParseKinds.td:56-58 +def warn_null_statement : Warning< + "';' with no preceding expression is a null statement">, + InGroup, DefaultIgnore; The warning should explain why we're warning: what the problem is, and why it warrants action from the user. I would expect people seeing this warning to think "yes, but so what?". Something like "statement has no effect" might be better, maybe? (A customized warning for `;;` might be nice too.) Repository: rC Clang https://reviews.llvm.org/D52695 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)
lebedev.ri updated this revision to Diff 168157.
lebedev.ri marked 4 inline comments as done.
lebedev.ri added a comment.
After looking at stage-2 of LLVM, remove `-Wextra-semi-stmt` from `-Wextra`,
and add `-Wempty-init-stmt` back into `-Wextra`.
Anything else?
Repository:
rC Clang
https://reviews.llvm.org/D52695
Files:
docs/ReleaseNotes.rst
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticParseKinds.td
lib/Parse/ParseExprCXX.cpp
lib/Parse/ParseStmt.cpp
test/Parser/extra-semi-resulting-in-nullstmt-in-init-statement.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,76 @@
+// RUN: %clang_cc1 -fsyntax-only -Wextra-semi-stmt -verify %s
+// RUN: cp %s %t
+// RUN: %clang_cc1 -x c++ -Wextra-semi-stmt -fixit %t
+// RUN: %clang_cc1 -x c++ -Wextra-semi-stmt -Werror %t
+
+#define GOODMACRO(varname) int varname
+#define BETTERMACRO(varname) GOODMACRO(varname);
+#define NULLMACRO(varname)
+
+enum MyEnum {
+ E1,
+ E2
+};
+
+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
+
+ MyEnum my_enum;
+ switch (my_enum) {
+ case E1:
+// stuff
+break;
+ case E2:; // OK
+ }
+
+ for (;;) {
+for (;;) {
+ goto contin_outer;
+}
+ contin_outer:; // OK
+ }
+}
Index: test/Parser/extra-semi-resulting-in-nullstmt-in-init-statement.cpp
===
--- /dev/null
+++ test/Parser/extra-semi-resulting-in-nullstmt-in-init-statement.cpp
@@ -0,0 +1,64 @@
+// RUN: %clang_cc1 -fsyntax-only -Wextra -std=c++2a -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wextra-semi-stmt -std=c++2a -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wempty-init-stmt -std=c++2a -verify %s
+// RUN: cp %s %t
+// RUN: %clang_cc1 -x c++ -Wempty-init-stmt -std=c++2a -fixit %t
+// RUN: %clang_cc1 -x c++ -Wempty-init-stmt -std=c++2a -Werror %t
+
+struct S {
+ int *begin();
+ int *end();
+};
+
+void naive(int x) {
+ if (; true) // expected-warning {{init-statement of 'if' is a null statement}}
+;
+
+ switch (; x) { // expected-warning {{init-statement of 'switch' is a null statement}}
+ }
+
+ for (; int y : S()) // expected-warning {{init-statement of 'range-based for' is a null statement}}
+;
+
+ for (;;) // OK
+;
+}
+
+#define NULLMACRO
+
+void with_null_macro(int x) {
+ if (NULLMACRO; true)
+;
+
+ switch (NULLMACRO; x) {
+ }
+
+ for (NULLMACRO; int y : S())
+;
+}
+
+#define SEMIMACRO ;
+
+void with_semi_macro(int x) {
+ if (SEMIMACRO true)
+;
+
+ switch (SEMIMACRO x) {
+ }
+
+ for (SEMIMACRO int y : S())
+;
+}
+
+#define PASSTHROUGHMACRO(x) x
+
+void with_passthrough_macro(int x) {
+ if (PASSTHROUGHMACRO(;) true)
+;
+
+ switch (PASSTHROUGHMACRO(;) x) {
+ }
+
+ for (PASSTHROUGHMACRO(;) int y : S())
+;
+}
Index: lib/Parse/ParseStmt.cpp
===
--- lib/Parse/ParseStmt.cpp
+++ lib/Parse/ParseStmt.cpp
@@ -992,6 +992,15 @@
continue;
}
+if (Tok.is(tok::semi)) {
+ bool HasLeadingEmptyMacro = Tok.hasLeadingEmptyMacro();
+ SourceLocation SemiLocation = Tok.getLocation();
+ if (!HasLeadingEmptyMacro && !SemiLocation.isMacroID()) {
+Diag(SemiLocation, diag::warn_null_statement)
+<< FixItHint::CreateRemoval(SemiLocation);
+ }
+}
+
StmtResult R;
if (Tok.isNot(tok::kw___extension__)) {
R = ParseStatementOrDeclaration(Stmts, ACK_Any);
@@ -1588,10 +1597,15 @@
ParsedAttributesWithRange attrs(AttrFactory);
MaybeParseCXX11Attributes(attrs);
+ SourceLocation EmptyInitStmtSemiLoc;
+
// Parse the first
[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)
lebedev.ri added inline comments. Comment at: include/clang/Basic/DiagnosticGroups.td:770 +NullPointerArithmetic, +ExtraSemiStmt ]>; I'm really unsure of this. Maybe this should only be `EmptyInitStatement`. Repository: rC Clang https://reviews.llvm.org/D52695 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)
lebedev.ri updated this revision to Diff 168020.
lebedev.ri added a comment.
Slightly improved test coverage for macros in
`extra-semi-resulting-in-nullstmt-in-init-statement.cpp`
Repository:
rC Clang
https://reviews.llvm.org/D52695
Files:
docs/ReleaseNotes.rst
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticParseKinds.td
lib/Parse/ParseExprCXX.cpp
lib/Parse/ParseStmt.cpp
test/Parser/extra-semi-resulting-in-nullstmt-in-init-statement.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,77 @@
+// RUN: %clang_cc1 -fsyntax-only -Wextra -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wextra-semi-stmt -verify %s
+// RUN: cp %s %t
+// RUN: %clang_cc1 -x c++ -Wextra-semi-stmt -fixit %t
+// RUN: %clang_cc1 -x c++ -Wextra-semi-stmt -Werror %t
+
+#define GOODMACRO(varname) int varname
+#define BETTERMACRO(varname) GOODMACRO(varname);
+#define NULLMACRO(varname)
+
+enum MyEnum {
+ E1,
+ E2
+};
+
+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
+
+ MyEnum my_enum;
+ switch (my_enum) {
+ case E1:
+// stuff
+break;
+ case E2:; // OK
+ }
+
+ for (;;) {
+for (;;) {
+ goto contin_outer;
+}
+ contin_outer:; // OK
+ }
+}
Index: test/Parser/extra-semi-resulting-in-nullstmt-in-init-statement.cpp
===
--- /dev/null
+++ test/Parser/extra-semi-resulting-in-nullstmt-in-init-statement.cpp
@@ -0,0 +1,64 @@
+// RUN: %clang_cc1 -fsyntax-only -Wextra -std=c++2a -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wextra-semi-stmt -std=c++2a -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wempty-init-stmt -std=c++2a -verify %s
+// RUN: cp %s %t
+// RUN: %clang_cc1 -x c++ -Wempty-init-stmt -std=c++2a -fixit %t
+// RUN: %clang_cc1 -x c++ -Wempty-init-stmt -std=c++2a -Werror %t
+
+struct S {
+ int *begin();
+ int *end();
+};
+
+void naive(int x) {
+ if (; true) // expected-warning {{init-statement of 'if' is a null statement}}
+;
+
+ switch (; x) { // expected-warning {{init-statement of 'switch' is a null statement}}
+ }
+
+ for (; int y : S()) // expected-warning {{init-statement of 'range-based for' is a null statement}}
+;
+
+ for (;;) // OK
+;
+}
+
+#define NULLMACRO
+
+void with_null_macro(int x) {
+ if (NULLMACRO; true)
+;
+
+ switch (NULLMACRO; x) {
+ }
+
+ for (NULLMACRO; int y : S())
+;
+}
+
+#define SEMIMACRO ;
+
+void with_semi_macro(int x) {
+ if (SEMIMACRO true)
+;
+
+ switch (SEMIMACRO x) {
+ }
+
+ for (SEMIMACRO int y : S())
+;
+}
+
+#define PASSTHROUGHMACRO(x) x
+
+void with_passthrough_macro(int x) {
+ if (PASSTHROUGHMACRO(;) true)
+;
+
+ switch (PASSTHROUGHMACRO(;) x) {
+ }
+
+ for (PASSTHROUGHMACRO(;) int y : S())
+;
+}
Index: lib/Parse/ParseStmt.cpp
===
--- lib/Parse/ParseStmt.cpp
+++ lib/Parse/ParseStmt.cpp
@@ -992,6 +992,15 @@
continue;
}
+if (Tok.is(tok::semi)) {
+ bool HasLeadingEmptyMacro = Tok.hasLeadingEmptyMacro();
+ SourceLocation SemiLocation = Tok.getLocation();
+ if (!HasLeadingEmptyMacro && !SemiLocation.isMacroID()) {
+Diag(SemiLocation, diag::warn_null_statement)
+<< FixItHint::CreateRemoval(SemiLocation);
+ }
+}
+
StmtResult R;
if (Tok.isNot(tok::kw___extension__)) {
R = ParseStatementOrDeclaration(Stmts, ACK_Any);
@@ -1588,10 +1597,15 @@
ParsedAttributesWithRange attrs(AttrFactory);
MaybeParseCXX11Attributes(attrs);
+ SourceLocation EmptyInitStmtSemiLoc;
+
// Parse the first part of the for specifier.
if
[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)
lebedev.ri added inline comments. Comment at: include/clang/Basic/DiagnosticGroups.td:167-168 + CXX11ExtraSemi, + ExtraSemiStmt, + EmptyInitStatement]>; rsmith wrote: > I would prefer to keep `-Wextra-semi` and `-Wextra-semi-stmt` separate. > `-Wextra-semi` is for those cases that are ill-formed in C++98, whereas your > new warning is only about code style / typo detection. Hmm, does that mean ExtraSemiStmt[+EmptyInitStatement] should be in `-Wextra`? Comment at: lib/Parse/ParseStmt.cpp:1600 + llvm::Optional EmptyInitStmtSemiLoc; + rsmith wrote: > You can just use a `SourceLocation` here; it already has an 'invalid' state. Aha. Repository: rC Clang https://reviews.llvm.org/D52695 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)
lebedev.ri updated this revision to Diff 168017.
lebedev.ri marked 3 inline comments as done.
lebedev.ri added a comment.
- Moved `-Wempty-init-stmt` into `-Wextra-semi-stmt`
- Moved `-Wextra-semi-stmt` out of `-Wextra-semi`
- Tentatively enabled `-Wextra-semi-stmt` in `-Wextra` (and removed
`-Wempty-init-stmt` from `-Wextra`, since it will be transitively enabled by
`-Wextra-semi-stmt`)
Repository:
rC Clang
https://reviews.llvm.org/D52695
Files:
docs/ReleaseNotes.rst
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticParseKinds.td
lib/Parse/ParseExprCXX.cpp
lib/Parse/ParseStmt.cpp
test/Parser/extra-semi-resulting-in-nullstmt-in-init-statement.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,77 @@
+// RUN: %clang_cc1 -fsyntax-only -Wextra -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wextra-semi-stmt -verify %s
+// RUN: cp %s %t
+// RUN: %clang_cc1 -x c++ -Wextra-semi-stmt -fixit %t
+// RUN: %clang_cc1 -x c++ -Wextra-semi-stmt -Werror %t
+
+#define GOODMACRO(varname) int varname
+#define BETTERMACRO(varname) GOODMACRO(varname);
+#define NULLMACRO(varname)
+
+enum MyEnum {
+ E1,
+ E2
+};
+
+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
+
+ MyEnum my_enum;
+ switch (my_enum) {
+ case E1:
+// stuff
+break;
+ case E2:; // OK
+ }
+
+ for (;;) {
+for (;;) {
+ goto contin_outer;
+}
+ contin_outer:; // OK
+ }
+}
Index: test/Parser/extra-semi-resulting-in-nullstmt-in-init-statement.cpp
===
--- /dev/null
+++ test/Parser/extra-semi-resulting-in-nullstmt-in-init-statement.cpp
@@ -0,0 +1,38 @@
+// RUN: %clang_cc1 -fsyntax-only -Wextra -std=c++2a -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wextra-semi-stmt -std=c++2a -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wempty-init-stmt -std=c++2a -verify %s
+// RUN: cp %s %t
+// RUN: %clang_cc1 -x c++ -Wempty-init-stmt -std=c++2a -fixit %t
+// RUN: %clang_cc1 -x c++ -Wempty-init-stmt -std=c++2a -Werror %t
+
+struct S {
+ int *begin();
+ int *end();
+};
+
+void naive(int x) {
+ if (; true) // expected-warning {{init-statement of 'if' is a null statement}}
+;
+
+ switch (; x) { // expected-warning {{init-statement of 'switch' is a null statement}}
+ }
+
+ for (; int y : S()) // expected-warning {{init-statement of 'range-based for' is a null statement}}
+;
+
+ for (;;) // OK
+;
+}
+
+#define NULLMACRO
+
+void with_null_macro(int x) {
+ if (NULLMACRO; true)
+;
+
+ switch (NULLMACRO; x) {
+ }
+
+ for (NULLMACRO; int y : S())
+;
+}
Index: lib/Parse/ParseStmt.cpp
===
--- lib/Parse/ParseStmt.cpp
+++ lib/Parse/ParseStmt.cpp
@@ -992,6 +992,15 @@
continue;
}
+if (Tok.is(tok::semi)) {
+ bool HasLeadingEmptyMacro = Tok.hasLeadingEmptyMacro();
+ SourceLocation SemiLocation = Tok.getLocation();
+ if (!HasLeadingEmptyMacro && !SemiLocation.isMacroID()) {
+Diag(SemiLocation, diag::warn_null_statement)
+<< FixItHint::CreateRemoval(SemiLocation);
+ }
+}
+
StmtResult R;
if (Tok.isNot(tok::kw___extension__)) {
R = ParseStatementOrDeclaration(Stmts, ACK_Any);
@@ -1588,10 +1597,15 @@
ParsedAttributesWithRange attrs(AttrFactory);
MaybeParseCXX11Attributes(attrs);
+ SourceLocation EmptyInitStmtSemiLoc;
+
// Parse the first part of the for specifier.
if (Tok.is(tok::semi)) { // for (;
ProhibitAttributes(attrs);
// no first part, eat the ';'.
+SourceLocation SemiLoc = Tok.getLocation();
+if
[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)
rsmith added inline comments. Comment at: include/clang/Basic/DiagnosticGroups.td:164 +def ExtraSemiStmt : DiagGroup<"extra-semi-stmt">; +def EmptyInitStatement : DiagGroup<"empty-init-stmt">; def ExtraSemi : DiagGroup<"extra-semi", [CXX98CompatExtraSemi, I think this should either be folded into `-Wextra-semi-stmt` or should perhaps be a subgroup of it. Comment at: include/clang/Basic/DiagnosticGroups.td:167-168 + CXX11ExtraSemi, + ExtraSemiStmt, + EmptyInitStatement]>; I would prefer to keep `-Wextra-semi` and `-Wextra-semi-stmt` separate. `-Wextra-semi` is for those cases that are ill-formed in C++98, whereas your new warning is only about code style / typo detection. Comment at: lib/Parse/ParseStmt.cpp:1600 + llvm::Optional EmptyInitStmtSemiLoc; + You can just use a `SourceLocation` here; it already has an 'invalid' state. Repository: rC Clang https://reviews.llvm.org/D52695 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)
lebedev.ri added inline comments.
Comment at: lib/Parse/ParseStmt.cpp:237
+SourceLocation SemiLocation = ConsumeToken();
+if (!HasLeadingEmptyMacro && getCurScope()->isCompoundStmtScope() &&
+!SemiLocation.isMacroID()) {
rsmith wrote:
> I'm a little concerned that checking whether the scope is a compound
> statement isn't really checking the right thing -- what you care about is
> whether the syntactic context is a compound statement, not which scope a
> declaration at this level would be injected into. (Example of the difference:
> back in the pre-standard times when `{ for (int x = 0; x < 10; ++x) //...`
> injected `x` into the enclosing scope, the first substatement in that `for`
> statement would be in a compound-statement scope, but we definitely shouldn't
> warn on a stray `;` there.) If you moved this check into
> `ParseCompoundStatementBody`, there'd be no risk of such problems.
Great idea, thank you!
Testcases added, that fixed them.
Comment at: test/Parser/extra-semi-resulting-in-nullstmt.cpp:59
+#if __cplusplus >= 201703L
+ if (; true) // OK
+; // OK
rsmith wrote:
> It'd seem reasonable to warn on a null-statement as the *init-statement* of
> an `if` / `switch` / range-based `for`.
If you so insist :)
Repository:
rC Clang
https://reviews.llvm.org/D52695
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)
lebedev.ri updated this revision to Diff 168000.
lebedev.ri marked 4 inline comments as done.
lebedev.ri added a comment.
Thank you for taking a look!
- Move it into `ParseCompoundStatementBody()`, thus fixing false-positives with
`case X: ;` e.g.
- Rename to `-Wextra-semi-stmt`
- Add `-Wempty-init-stmt` (tentatively included into `-Wextra` too), to
diagnose empty init-statements of `if`/`switch`/`range-based for`
Repository:
rC Clang
https://reviews.llvm.org/D52695
Files:
docs/ReleaseNotes.rst
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticParseKinds.td
lib/Parse/ParseExprCXX.cpp
lib/Parse/ParseStmt.cpp
test/Parser/extra-semi-resulting-in-nullstmt.cpp
test/SemaCXX/extra-semi-resulting-in-nullstmt-in-init-statement.cpp
Index: test/SemaCXX/extra-semi-resulting-in-nullstmt-in-init-statement.cpp
===
--- /dev/null
+++ test/SemaCXX/extra-semi-resulting-in-nullstmt-in-init-statement.cpp
@@ -0,0 +1,38 @@
+// RUN: %clang_cc1 -fsyntax-only -Wextra -std=c++2a -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wextra-semi -std=c++2a -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wempty-init-stmt -std=c++2a -verify %s
+// RUN: cp %s %t
+// RUN: %clang_cc1 -x c++ -Wempty-init-stmt -std=c++2a -fixit %t
+// RUN: %clang_cc1 -x c++ -Wempty-init-stmt -std=c++2a -Werror %t
+
+struct S {
+ int *begin();
+ int *end();
+};
+
+void naive(int x) {
+ if (; true) // expected-warning {{init-statement of 'if' is a null statement}}
+;
+
+ switch (; x) { // expected-warning {{init-statement of 'switch' is a null statement}}
+ }
+
+ for (; int y : S()) // expected-warning {{init-statement of 'range-based for' is a null statement}}
+;
+
+ for (;;) // OK
+;
+}
+
+#define NULLMACRO
+
+void with_null_macro(int x) {
+ if (NULLMACRO; true)
+;
+
+ switch (NULLMACRO; x) {
+ }
+
+ for (NULLMACRO; int y : S())
+;
+}
Index: test/Parser/extra-semi-resulting-in-nullstmt.cpp
===
--- /dev/null
+++ test/Parser/extra-semi-resulting-in-nullstmt.cpp
@@ -0,0 +1,76 @@
+// RUN: %clang_cc1 -fsyntax-only -Wextra-semi-stmt -verify %s
+// RUN: cp %s %t
+// RUN: %clang_cc1 -x c++ -Wextra-semi-stmt -fixit %t
+// RUN: %clang_cc1 -x c++ -Wextra-semi-stmt -Werror %t
+
+#define GOODMACRO(varname) int varname
+#define BETTERMACRO(varname) GOODMACRO(varname);
+#define NULLMACRO(varname)
+
+enum MyEnum {
+ E1,
+ E2
+};
+
+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
+
+ MyEnum my_enum;
+ switch (my_enum) {
+ case E1:
+// stuff
+break;
+ case E2:; // OK
+ }
+
+ for (;;) {
+for (;;) {
+ goto contin_outer;
+}
+ contin_outer:; // OK
+ }
+}
Index: lib/Parse/ParseStmt.cpp
===
--- lib/Parse/ParseStmt.cpp
+++ lib/Parse/ParseStmt.cpp
@@ -992,6 +992,15 @@
continue;
}
+if (Tok.is(tok::semi)) {
+ bool HasLeadingEmptyMacro = Tok.hasLeadingEmptyMacro();
+ SourceLocation SemiLocation = Tok.getLocation();
+ if (!HasLeadingEmptyMacro && !SemiLocation.isMacroID()) {
+Diag(SemiLocation, diag::warn_null_statement)
+<< FixItHint::CreateRemoval(SemiLocation);
+ }
+}
+
StmtResult R;
if (Tok.isNot(tok::kw___extension__)) {
R = ParseStatementOrDeclaration(Stmts, ACK_Any);
@@ -1588,10 +1597,15 @@
ParsedAttributesWithRange attrs(AttrFactory);
MaybeParseCXX11Attributes(attrs);
+ llvm::Optional EmptyInitStmtSemiLoc;
+
// Parse the first part of the for specifier.
if (Tok.is(tok::semi)) { // for (;
ProhibitAttributes(attrs);
// no first part, eat the ';'.
+SourceLocation SemiLoc = Tok.getLocation();
+if (!Tok.hasLeadingEmptyMacro()
[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)
rsmith added a comment.
Like Eli, I don't see much value in this warning. But it doesn't seem much
different from `-Wextra-semi` in that regard for languages in which such extra
semicolons are generally valid -- I expect both warnings will generally
diagnose typos and little else.
Does this warn on:
switch (my_enum) {
case E1:
// stuff
break;
case E2:
; // nothing to do, but a label requires a following statement
}
and
for (/*...outer...*/) {
for (/*...inner...*/) {
goto contin_outer;
}
contin_outer: ;
}
? We shouldn't warn on a semicolon after a label, because a statement is
required there, and use of a null statement is idiomatic.
Comment at: include/clang/Basic/DiagnosticGroups.td:163
def CXX11ExtraSemi : DiagGroup<"c++11-extra-semi">;
+def ExtraSemiPedantic : DiagGroup<"extra-semi-pedantic">;
def ExtraSemi : DiagGroup<"extra-semi", [CXX98CompatExtraSemi,
efriedma wrote:
> We usually only use "pedantic" in the names of warnings which are enabled by
> the "-pedantic" flag.
Maybe `-Wextra-semi-stmt` or `-Wredundant-null-stmt` or similar would be more
appropriate?
Comment at: lib/Parse/ParseStmt.cpp:237
+SourceLocation SemiLocation = ConsumeToken();
+if (!HasLeadingEmptyMacro && getCurScope()->isCompoundStmtScope() &&
+!SemiLocation.isMacroID()) {
I'm a little concerned that checking whether the scope is a compound statement
isn't really checking the right thing -- what you care about is whether the
syntactic context is a compound statement, not which scope a declaration at
this level would be injected into. (Example of the difference: back in the
pre-standard times when `{ for (int x = 0; x < 10; ++x) //...` injected `x`
into the enclosing scope, the first substatement in that `for` statement would
be in a compound-statement scope, but we definitely shouldn't warn on a stray
`;` there.) If you moved this check into `ParseCompoundStatementBody`, there'd
be no risk of such problems.
Comment at: test/Parser/extra-semi-resulting-in-nullstmt.cpp:59
+#if __cplusplus >= 201703L
+ if (; true) // OK
+; // OK
It'd seem reasonable to warn on a null-statement as the *init-statement* of an
`if` / `switch` / range-based `for`.
Repository:
rC Clang
https://reviews.llvm.org/D52695
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)
efriedma added a comment. Again, I'm not sure we really want this... we don't really like adding new off-by-default warnings, and we don't usually add diagnostics to enforce style. Comment at: include/clang/Basic/DiagnosticGroups.td:163 def CXX11ExtraSemi : DiagGroup<"c++11-extra-semi">; +def ExtraSemiPedantic : DiagGroup<"extra-semi-pedantic">; def ExtraSemi : DiagGroup<"extra-semi", [CXX98CompatExtraSemi, We usually only use "pedantic" in the names of warnings which are enabled by the "-pedantic" flag. Repository: rC Clang https://reviews.llvm.org/D52695 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)
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
;;; <- PS, 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
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, DefaultIgnore;
+def warn_null_statement : Warning<
+ "';' with no preceding expression is a null statement">,
+ InGroup, 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
+++
