[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)
Richard Smith - zygoloid via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Oct 1 15:49:13 PDT 2018
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
More information about the cfe-commits
mailing list