Author: Tyker Date: 2019-12-03T21:21:27+01:00 New Revision: bc840b21e1612adb603b6c17be0329e3737bb943
URL: https://github.com/llvm/llvm-project/commit/bc840b21e1612adb603b6c17be0329e3737bb943 DIFF: https://github.com/llvm/llvm-project/commit/bc840b21e1612adb603b6c17be0329e3737bb943.diff LOG: [Diagnostic] add a warning which warns about misleading indentation Summary: Add a warning for misleading indentation similar to GCC's -Wmisleading-indentation Reviewers: aaron.ballman, xbolva00 Reviewed By: aaron.ballman, xbolva00 Subscribers: tstellar, cfe-commits, arphaman, Ka-Ka, thakis Tags: #clang Differential Revision: https://reviews.llvm.org/D70638 Added: clang/test/Parser/warn-misleading-indentation.cpp Modified: clang/include/clang/Basic/DiagnosticGroups.td clang/include/clang/Basic/DiagnosticParseKinds.td clang/include/clang/Lex/Preprocessor.h clang/include/clang/Parse/Parser.h clang/lib/Parse/ParseStmt.cpp clang/test/Index/pragma-diag-reparse.c clang/test/Misc/warning-wall.c clang/test/Preprocessor/pragma_diagnostic_sections.cpp Removed: ################################################################################ diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td index 35e939fda95c..478b217a19f6 100644 --- a/clang/include/clang/Basic/DiagnosticGroups.td +++ b/clang/include/clang/Basic/DiagnosticGroups.td @@ -693,6 +693,7 @@ def ZeroLengthArray : DiagGroup<"zero-length-array">; def GNUZeroLineDirective : DiagGroup<"gnu-zero-line-directive">; def GNUZeroVariadicMacroArguments : DiagGroup<"gnu-zero-variadic-macro-arguments">; def Fallback : DiagGroup<"fallback">; +def MisleadingIndentation : DiagGroup<"misleading-indentation">; // This covers both the deprecated case (in C++98) // and the extension case (in C++11 onwards). @@ -884,7 +885,7 @@ def Consumed : DiagGroup<"consumed">; // Note that putting warnings in -Wall will not disable them by default. If a // warning should be active _only_ when -Wall is passed in, mark it as // DefaultIgnore in addition to putting it here. -def All : DiagGroup<"all", [Most, Parentheses, Switch, SwitchBool]>; +def All : DiagGroup<"all", [Most, Parentheses, Switch, SwitchBool, MisleadingIndentation]>; // Warnings that should be in clang-cl /w4. def : DiagGroup<"CL4", [All, Extra]>; diff --git a/clang/include/clang/Basic/DiagnosticParseKinds.td b/clang/include/clang/Basic/DiagnosticParseKinds.td index c94d1b99d0e8..8643e1b867f7 100644 --- a/clang/include/clang/Basic/DiagnosticParseKinds.td +++ b/clang/include/clang/Basic/DiagnosticParseKinds.td @@ -61,6 +61,13 @@ def warn_null_statement : Warning< "remove unnecessary ';' to silence this warning">, InGroup<ExtraSemiStmt>, DefaultIgnore; +def warn_misleading_indentation : Warning< + "misleading indentation; statement is not part of " + "the previous '%select{if|else|for|while|else if}0'">, + InGroup<MisleadingIndentation>, DefaultIgnore; +def note_previous_statement : Note< + "previous statement is here">; + def ext_thread_before : Extension<"'__thread' before '%0'">; def ext_keyword_as_ident : ExtWarn< "keyword '%0' will be made available as an identifier " diff --git a/clang/include/clang/Lex/Preprocessor.h b/clang/include/clang/Lex/Preprocessor.h index e2ddc80d503f..9716196b95c2 100644 --- a/clang/include/clang/Lex/Preprocessor.h +++ b/clang/include/clang/Lex/Preprocessor.h @@ -932,6 +932,12 @@ class Preprocessor { return TheModuleLoader.HadFatalFailure; } + /// Retrieve the number of Directives that have been processed by the + /// Preprocessor. + unsigned getNumDirectives() const { + return NumDirectives; + } + /// True if we are currently preprocessing a #if or #elif directive bool isParsingIfOrElifDirective() const { return ParsingIfOrElifDirective; diff --git a/clang/include/clang/Parse/Parser.h b/clang/include/clang/Parse/Parser.h index ea1116ff7a23..a1e7bbba9b8e 100644 --- a/clang/include/clang/Parse/Parser.h +++ b/clang/include/clang/Parse/Parser.h @@ -1122,6 +1122,11 @@ class Parser : public CodeCompletionHandler { /// point for skipping past a simple-declaration. void SkipMalformedDecl(); + /// The location of the first statement inside an else that might + /// have a missleading indentation. If there is no + /// MisleadingIndentationChecker on an else active, this location is invalid. + SourceLocation MisleadingIndentationElseLoc; + private: //===--------------------------------------------------------------------===// // Lexing and parsing of C++ inline methods. diff --git a/clang/lib/Parse/ParseStmt.cpp b/clang/lib/Parse/ParseStmt.cpp index 727ab75adae8..dc951dc22f55 100644 --- a/clang/lib/Parse/ParseStmt.cpp +++ b/clang/lib/Parse/ParseStmt.cpp @@ -1191,6 +1191,59 @@ bool Parser::ParseParenExprOrCondition(StmtResult *InitStmt, return false; } +namespace { + +enum MisleadingStatementKind { MSK_if, MSK_else, MSK_for, MSK_while }; + +struct MisleadingIndentationChecker { + Parser &P; + SourceLocation StmtLoc; + SourceLocation PrevLoc; + unsigned NumDirectives; + MisleadingStatementKind Kind; + bool NeedsChecking; + bool ShouldSkip; + MisleadingIndentationChecker(Parser &P, MisleadingStatementKind K, + SourceLocation SL) + : P(P), StmtLoc(SL), PrevLoc(P.getCurToken().getLocation()), + NumDirectives(P.getPreprocessor().getNumDirectives()), Kind(K), + NeedsChecking(true), ShouldSkip(P.getCurToken().is(tok::l_brace)) { + if (!P.MisleadingIndentationElseLoc.isInvalid()) { + StmtLoc = P.MisleadingIndentationElseLoc; + P.MisleadingIndentationElseLoc = SourceLocation(); + } + if (Kind == MSK_else && !ShouldSkip) + P.MisleadingIndentationElseLoc = SL; + } + void Check() { + NeedsChecking = false; + Token Tok = P.getCurToken(); + if (ShouldSkip || NumDirectives != P.getPreprocessor().getNumDirectives() || + Tok.isOneOf(tok::semi, tok::r_brace) || Tok.isAnnotation() || + Tok.getLocation().isMacroID() || PrevLoc.isMacroID() || + StmtLoc.isMacroID() || + (Kind == MSK_else && P.MisleadingIndentationElseLoc.isInvalid())) { + P.MisleadingIndentationElseLoc = SourceLocation(); + return; + } + + SourceManager &SM = P.getPreprocessor().getSourceManager(); + unsigned PrevColNum = SM.getSpellingColumnNumber(PrevLoc); + unsigned CurColNum = SM.getSpellingColumnNumber(Tok.getLocation()); + unsigned StmtColNum = SM.getSpellingColumnNumber(StmtLoc); + + if (PrevColNum != 0 && CurColNum != 0 && StmtColNum != 0 && + ((PrevColNum > StmtColNum && PrevColNum == CurColNum) || + !Tok.isAtStartOfLine()) && SM.getPresumedLineNumber(StmtLoc) != + SM.getPresumedLineNumber(Tok.getLocation())) { + P.Diag(Tok.getLocation(), diag::warn_misleading_indentation) + << Kind; + P.Diag(StmtLoc, diag::note_previous_statement); + } + } +}; + +} /// ParseIfStatement /// if-statement: [C99 6.8.4.1] @@ -1265,6 +1318,8 @@ StmtResult Parser::ParseIfStatement(SourceLocation *TrailingElseLoc) { // ParseScope InnerScope(this, Scope::DeclScope, C99orCXX, Tok.is(tok::l_brace)); + MisleadingIndentationChecker MIChecker(*this, MSK_if, IfLoc); + // Read the 'then' stmt. SourceLocation ThenStmtLoc = Tok.getLocation(); @@ -1278,6 +1333,9 @@ StmtResult Parser::ParseIfStatement(SourceLocation *TrailingElseLoc) { ThenStmt = ParseStatement(&InnerStatementTrailingElseLoc); } + if (Tok.isNot(tok::kw_else)) + MIChecker.Check(); + // Pop the 'if' scope if needed. InnerScope.Exit(); @@ -1305,12 +1363,17 @@ StmtResult Parser::ParseIfStatement(SourceLocation *TrailingElseLoc) { ParseScope InnerScope(this, Scope::DeclScope, C99orCXX, Tok.is(tok::l_brace)); + MisleadingIndentationChecker MIChecker(*this, MSK_else, ElseLoc); + EnterExpressionEvaluationContext PotentiallyDiscarded( Actions, Sema::ExpressionEvaluationContext::DiscardedStatement, nullptr, Sema::ExpressionEvaluationContextRecord::EK_Other, /*ShouldEnter=*/ConstexprCondition && *ConstexprCondition); ElseStmt = ParseStatement(); + if (ElseStmt.isUsable()) + MIChecker.Check(); + // Pop the 'else' scope if needed. InnerScope.Exit(); } else if (Tok.is(tok::code_completion)) { @@ -1484,9 +1547,13 @@ StmtResult Parser::ParseWhileStatement(SourceLocation *TrailingElseLoc) { // ParseScope InnerScope(this, Scope::DeclScope, C99orCXX, Tok.is(tok::l_brace)); + MisleadingIndentationChecker MIChecker(*this, MSK_while, WhileLoc); + // Read the body statement. StmtResult Body(ParseStatement(TrailingElseLoc)); + if (Body.isUsable()) + MIChecker.Check(); // Pop the body scope if needed. InnerScope.Exit(); WhileScope.Exit(); @@ -1918,9 +1985,14 @@ StmtResult Parser::ParseForStatement(SourceLocation *TrailingElseLoc) { if (C99orCXXorObjC) getCurScope()->decrementMSManglingNumber(); + MisleadingIndentationChecker MIChecker(*this, MSK_for, ForLoc); + // Read the body statement. StmtResult Body(ParseStatement(TrailingElseLoc)); + if (Body.isUsable()) + MIChecker.Check(); + // Pop the body scope if needed. InnerScope.Exit(); diff --git a/clang/test/Index/pragma-diag-reparse.c b/clang/test/Index/pragma-diag-reparse.c index 71d0618d7092..aa1413cda089 100644 --- a/clang/test/Index/pragma-diag-reparse.c +++ b/clang/test/Index/pragma-diag-reparse.c @@ -11,6 +11,7 @@ int main (int argc, const char * argv[]) return x; } +#pragma clang diagnostic ignored "-Wmisleading-indentation" void foo() { int b=0; while (b==b); } // RUN: env CINDEXTEST_EDITING=1 CINDEXTEST_FAILONERROR=1 c-index-test -test-load-source-reparse 5 local \ diff --git a/clang/test/Misc/warning-wall.c b/clang/test/Misc/warning-wall.c index fadcceefe297..2b27b67eafa1 100644 --- a/clang/test/Misc/warning-wall.c +++ b/clang/test/Misc/warning-wall.c @@ -90,6 +90,7 @@ CHECK-NEXT: -Wparentheses-equality CHECK-NEXT: -Wdangling-else CHECK-NEXT: -Wswitch CHECK-NEXT: -Wswitch-bool +CHECK-NEXT: -Wmisleading-indentation CHECK-NOT:-W diff --git a/clang/test/Parser/warn-misleading-indentation.cpp b/clang/test/Parser/warn-misleading-indentation.cpp new file mode 100644 index 000000000000..e5ed8bba93c1 --- /dev/null +++ b/clang/test/Parser/warn-misleading-indentation.cpp @@ -0,0 +1,208 @@ +// RUN: %clang_cc1 -x c -fsyntax-only -verify %s +// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wmisleading-indentation -DWITH_WARN %s +// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused -DWITH_WARN -DCXX17 %s +// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused -Wno-misleading-indentation -DCXX17 %s + +#ifndef WITH_WARN +// expected-no-diagnostics +#endif + +void f0(int i) { + if (i) +#ifdef WITH_WARN +// expected-note@-2 {{here}} +#endif + i = i + 1; + int x = 0; +#ifdef WITH_WARN +// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}} +#endif + return; +#ifdef CXX17 + if constexpr (false) +#ifdef WITH_WARN +// expected-note@-2 {{here}} +#endif + i = 0; + i += 1; +#ifdef WITH_WARN +// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}} +#endif +#endif +} + +void f1(int i) { + for (;i;) +#ifdef WITH_WARN +// expected-note@-2 {{here}} +#endif + i = i + 1; + i *= 2; +#ifdef WITH_WARN +// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'for'}} +#endif + return; +} + +void f2(int i) { + while (i) +#ifdef WITH_WARN +// expected-note@-2 {{here}} +#endif + i = i + 1; i *= 2; +#ifdef WITH_WARN +// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'while'}} +#endif + return; +} + +void f3(int i) { + if (i) + i = i + 1; + else +#ifdef WITH_WARN +// expected-note@-2 {{here}} +#endif + i *= 2; + const int x = 0; +#ifdef WITH_WARN +// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'else'}} +#endif +} + +#ifdef CXX17 +struct Range { + int *begin() {return nullptr;} + int *end() {return nullptr;} +}; +#endif + +void f4(int i) { + if (i) + i *= 2; + return; + if (i) + i *= 2; + ; + if (i) +#ifdef WITH_WARN +// expected-note@-2 {{here}} +#endif + i *= 2; + typedef int Int; +#ifdef WITH_WARN +// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}} +#endif +#ifdef CXX17 + Range R; + for (auto e : R) +#ifdef WITH_WARN +// expected-note@-2 {{here}} +#endif + i *= 2; + using Int2 = int; +#ifdef WITH_WARN +// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'for'}} +#endif +#endif +} + +int bar(void); + +int foo(int* dst) +{ + if (dst) + return + bar(); + if (dst) + dst = dst + \ + bar(); + return 0; +} + +void g(int i) { + if (1) + i = 2; + else + if (i == 3) +#ifdef WITH_WARN +// expected-note@-3 {{here}} +#endif + i = 4; + i = 5; +#ifdef WITH_WARN +// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}} +#endif +} + +// Or this +#define TEST i = 5 +void g0(int i) { + if (1) + i = 2; + else + i = 5; + TEST; +} + +void g1(int i) { + if (1) + i = 2; + else if (i == 3) +#ifdef WITH_WARN +// expected-note@-2 {{here}} +#endif + i = 4; + i = 5; +#ifdef WITH_WARN +// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}} +#endif +} + +void g2(int i) { + if (1) + i = 2; + else + if (i == 3) + {i = 4;} + i = 5; +} + +void g6(int i) { + if (1) + if (i == 3) +#ifdef WITH_WARN +// expected-note@-2 {{here}} +#endif + i = 4; + i = 5; +#ifdef WITH_WARN +// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}} +#endif +} + +void g7(int i) { + if (1) + i = 4; +#ifdef TEST1 +#endif + i = 5; +} + +void a1(int i) { if (1) i = 4; return; } + +void a2(int i) { + { + if (1) + i = 4; + } + return; +} + +void a3(int i) { + if (1) + { + i = 4; + } + return; +} \ No newline at end of file diff --git a/clang/test/Preprocessor/pragma_diagnostic_sections.cpp b/clang/test/Preprocessor/pragma_diagnostic_sections.cpp index b680fae5b993..2bba7595a810 100644 --- a/clang/test/Preprocessor/pragma_diagnostic_sections.cpp +++ b/clang/test/Preprocessor/pragma_diagnostic_sections.cpp @@ -1,4 +1,4 @@ -// RUN: %clang_cc1 -fsyntax-only -Wall -Wunused-macros -Wunused-parameter -Wno-uninitialized -verify %s +// RUN: %clang_cc1 -fsyntax-only -Wall -Wunused-macros -Wunused-parameter -Wno-uninitialized -Wno-misleading-indentation -verify %s // rdar://8365684 struct S { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits