[clang] 7b86188 - [Diagnostic] add a warning which warns about misleading indentation
Maxim Kuvyrkov via cfe-commits
cfe-commits at lists.llvm.org
Tue Nov 26 02:15:39 PST 2019
Hi Tyker,
This patch generates lots of warnings on building Linux kernel, which seem to be false positives.
E.g.,
===
static inline bool
atomic_inc_unless_negative(atomic_t *v)
{
int c = atomic_read(v);
do {
if (unlikely(c < 0))
return false;
} while (!atomic_try_cmpxchg(v, &c, c + 1));
return true;
}
===
00:01:57 ./include/linux/atomic-fallback.h:1136:10: warning: misleading indentation; statement is not part of the previous 'else' [-Wmisleading-indentation]
00:01:57 int c = atomic_read(v);
00:01:57 ^
Tyker, would you please revert your patch (it generates multi-gig log files in CI builds) and investigate the problem?
Arnd, would you please help Tyker troubleshooting problems from kernel side?
Thank you,
--
Maxim Kuvyrkov
https://www.linaro.org
> On Nov 25, 2019, at 10:52 PM, via cfe-commits <cfe-commits at lists.llvm.org> wrote:
>
>
> Author: Tyker
> Date: 2019-11-25T20:46:32+01:00
> New Revision: 7b86188b50bf6e537fe98b326f258fbd23108b83
>
> URL: https://github.com/llvm/llvm-project/commit/7b86188b50bf6e537fe98b326f258fbd23108b83
> DIFF: https://github.com/llvm/llvm-project/commit/7b86188b50bf6e537fe98b326f258fbd23108b83.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: arphaman, Ka-Ka, thakis
>
> Differential Revision: https://reviews.llvm.org/D70638
>
> Added:
> clang/test/SemaCXX/warn-misleading-indentation.cpp
>
> Modified:
> clang/include/clang/Basic/DiagnosticGroups.td
> clang/include/clang/Basic/DiagnosticParseKinds.td
> 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 5bfb3de86a47..666193e074f0 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..e6aa92eddef7 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; %select{statement|declaration}0 is not part of "
> + "the previous '%select{if|else|for|while}1'">,
> + 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/Parse/Parser.h b/clang/include/clang/Parse/Parser.h
> index ea1116ff7a23..edd31e3ff7e8 100644
> --- a/clang/include/clang/Parse/Parser.h
> +++ b/clang/include/clang/Parse/Parser.h
> @@ -2266,11 +2266,13 @@ class Parser : public CodeCompletionHandler {
> return isTypeSpecifierQualifier();
> }
>
> +public:
> /// isCXXDeclarationStatement - C++-specialized function that disambiguates
> /// between a declaration or an expression statement, when parsing function
> /// bodies. Returns true for declaration, false for expression.
> bool isCXXDeclarationStatement();
>
> +private:
> /// isCXXSimpleDeclaration - C++-specialized function that disambiguates
> /// between a simple-declaration or an expression-statement.
> /// If during the disambiguation process a parsing error is encountered,
>
> diff --git a/clang/lib/Parse/ParseStmt.cpp b/clang/lib/Parse/ParseStmt.cpp
> index 727ab75adae8..ce8aa7574b9b 100644
> --- a/clang/lib/Parse/ParseStmt.cpp
> +++ b/clang/lib/Parse/ParseStmt.cpp
> @@ -1191,6 +1191,27 @@ bool Parser::ParseParenExprOrCondition(StmtResult *InitStmt,
> return false;
> }
>
> +enum MisleadingStatementKind { MSK_if, MSK_else, MSK_for, MSK_while };
> +
> +static void
> +MaybeDiagnoseMisleadingIndentation(Parser &P, SourceLocation PrevLoc,
> + SourceLocation StmtLoc,
> + MisleadingStatementKind StmtKind) {
> + Token Tok = P.getCurToken();
> + if (Tok.is(tok::semi))
> + return;
> + SourceManager &SM = P.getPreprocessor().getSourceManager();
> + unsigned PrevColNum = SM.getSpellingColumnNumber(PrevLoc);
> + unsigned CurColNum = SM.getSpellingColumnNumber(Tok.getLocation());
> + unsigned StmtColNum = SM.getSpellingColumnNumber(StmtLoc);
> + if (!Tok.isAtStartOfLine() ||
> + (PrevColNum != 0 && CurColNum != 0 && StmtColNum != 0 &&
> + PrevColNum > StmtColNum && PrevColNum == CurColNum)) {
> + P.Diag(Tok.getLocation(), diag::warn_misleading_indentation)
> + << P.isCXXDeclarationStatement() << StmtKind;
> + P.Diag(StmtLoc, diag::note_previous_statement);
> + }
> +}
>
> /// ParseIfStatement
> /// if-statement: [C99 6.8.4.1]
> @@ -1281,6 +1302,9 @@ StmtResult Parser::ParseIfStatement(SourceLocation *TrailingElseLoc) {
> // Pop the 'if' scope if needed.
> InnerScope.Exit();
>
> + if (Tok.isNot(tok::kw_else))
> + MaybeDiagnoseMisleadingIndentation(*this, ThenStmtLoc, IfLoc, MSK_if);
> +
> // If it has an else, parse it.
> SourceLocation ElseLoc;
> SourceLocation ElseStmtLoc;
> @@ -1313,6 +1337,9 @@ StmtResult Parser::ParseIfStatement(SourceLocation *TrailingElseLoc) {
>
> // Pop the 'else' scope if needed.
> InnerScope.Exit();
> + if (ElseStmt.isUsable())
> + MaybeDiagnoseMisleadingIndentation(*this, ElseStmt.get()->getBeginLoc(),
> + ElseLoc, MSK_else);
> } else if (Tok.is(tok::code_completion)) {
> Actions.CodeCompleteAfterIf(getCurScope());
> cutOffParsing();
> @@ -1491,6 +1518,10 @@ StmtResult Parser::ParseWhileStatement(SourceLocation *TrailingElseLoc) {
> InnerScope.Exit();
> WhileScope.Exit();
>
> + if (Body.isUsable())
> + MaybeDiagnoseMisleadingIndentation(*this, Body.get()->getBeginLoc(),
> + WhileLoc, MSK_while);
> +
> if (Cond.isInvalid() || Body.isInvalid())
> return StmtError();
>
> @@ -1927,6 +1958,10 @@ StmtResult Parser::ParseForStatement(SourceLocation *TrailingElseLoc) {
> // Leave the for-scope.
> ForScope.Exit();
>
> + if (Body.isUsable())
> + MaybeDiagnoseMisleadingIndentation(*this, Body.get()->getBeginLoc(), ForLoc,
> + MSK_for);
> +
> if (Body.isInvalid())
> return StmtError();
>
>
> 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/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 {
>
> diff --git a/clang/test/SemaCXX/warn-misleading-indentation.cpp b/clang/test/SemaCXX/warn-misleading-indentation.cpp
> new file mode 100644
> index 000000000000..fef83f1e0277
> --- /dev/null
> +++ b/clang/test/SemaCXX/warn-misleading-indentation.cpp
> @@ -0,0 +1,184 @@
> +// 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 at -2 {{here}}
> +#endif
> + i = i + 1;
> + int x = 0;
> +#ifdef WITH_WARN
> +// expected-warning at -2 {{misleading indentation; declaration is not part of the previous 'if'}}
> +#endif
> + return;
> +#ifdef CXX17
> + if constexpr (false)
> +#ifdef WITH_WARN
> +// expected-note at -2 {{here}}
> +#endif
> + i = 0;
> + i += 1;
> +#ifdef WITH_WARN
> +// expected-warning at -2 {{misleading indentation; statement is not part of the previous 'if'}}
> +#endif
> +#endif
> +}
> +
> +void f1(int i) {
> + for (;i;)
> +#ifdef WITH_WARN
> +// expected-note at -2 {{here}}
> +#endif
> + i = i + 1;
> + i *= 2;
> +#ifdef WITH_WARN
> +// expected-warning at -2 {{misleading indentation; statement is not part of the previous 'for'}}
> +#endif
> + return;
> +}
> +
> +void f2(int i) {
> + while (i)
> +#ifdef WITH_WARN
> +// expected-note at -2 {{here}}
> +#endif
> + i = i + 1; i *= 2;
> +#ifdef WITH_WARN
> +// expected-warning at -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 at -2 {{here}}
> +#endif
> + i *= 2;
> + const int x = 0;
> +#ifdef WITH_WARN
> +// expected-warning at -2 {{misleading indentation; declaration 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 at -2 {{here}}
> +#endif
> + i *= 2;
> + typedef int Int;
> +#ifdef WITH_WARN
> +// expected-warning at -2 {{misleading indentation; declaration is not part of the previous 'if'}}
> +#endif
> +#ifdef CXX17
> + Range R;
> + for (auto e : R)
> +#ifdef WITH_WARN
> +// expected-note at -2 {{here}}
> +#endif
> + i *= 2;
> + using Int2 = int;
> +#ifdef WITH_WARN
> +// expected-warning at -2 {{misleading indentation; declaration 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;
> +}
> +
> +// No diagnostics from GCC on this
> +void g(int i) {
> + if (1)
> + i = 2;
> + else
> + if (i == 3)
> + i = 4;
> + i = 5;
> +}
> +
> +// 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
> +#ifdef WITH_WARN
> +// expected-note at -2 {{here}}
> +#endif
> + if (i == 3)
> + i = 4;
> + i = 5;
> +#ifdef WITH_WARN
> +// expected-warning at -2 {{misleading indentation; statement is not part of the previous 'else'}}
> +#endif
> +}
> +
> +void g2(int i) {
> + if (1)
> + i = 2;
> + else
> +#ifdef WITH_WARN
> +// expected-note at -2 {{here}}
> +#endif
> + if (i == 3)
> + {i = 4;}
> + i = 5;
> +#ifdef WITH_WARN
> +// expected-warning at -2 {{misleading indentation; statement is not part of the previous 'else'}}
> +#endif
> +}
> +
> +void g6(int i) {
> + if (1)
> + if (i == 3)
> +#ifdef WITH_WARN
> +// expected-note at -2 {{here}}
> +#endif
> + i = 4;
> + i = 5;
> +#ifdef WITH_WARN
> +// expected-warning at -2 {{misleading indentation; statement is not part of the previous 'if'}}
> +#endif
> +}
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
More information about the cfe-commits
mailing list