Re: [PATCH] D16535: [clang-tidy] Check to find unintended semicolons that changes the semantics.
hokein added inline comments. Comment at: clang-tidy/misc/SuspiciousSemicolonCheck.cpp:23 @@ +22,3 @@ + Finder->addMatcher( + stmt(anyOf(ifStmt(hasThen(nullStmt().bind("semi"))), + forStmt(hasBody(nullStmt().bind("semi"))), Looks like this check doesn't handle the case that unintended semicolon is in `else` statement. ``` if (condition1) { } else if (condition2); a = 2 ``` Comment at: test/clang-tidy/misc-suspicious-semicolon.cpp:28 @@ +27,3 @@ +{ + if(x > 5); nop(); + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: potentially unintended semicolon [misc-suspicious-semicolon] Can you add the following `if` statement cases in the test? ``` if (condition) ; ``` ``` if (condition) ; else { } ``` The behavior of the check is that both two cases are not warned. But I think we should warn the first one since there is no reason to write `if` code for that. http://reviews.llvm.org/D16535 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16535: [clang-tidy] Check to find unintended semicolons that changes the semantics.
xazax.hun updated this revision to Diff 47316. xazax.hun added a comment. - Fixed the cases pointed out by the review. http://reviews.llvm.org/D16535 Files: clang-tidy/misc/CMakeLists.txt clang-tidy/misc/MiscTidyModule.cpp clang-tidy/misc/SuspiciousSemicolonCheck.cpp clang-tidy/misc/SuspiciousSemicolonCheck.h docs/clang-tidy/checks/list.rst docs/clang-tidy/checks/misc-suspicious-semicolon.rst test/clang-tidy/misc-suspicious-semicolon.cpp Index: test/clang-tidy/misc-suspicious-semicolon.cpp === --- /dev/null +++ test/clang-tidy/misc-suspicious-semicolon.cpp @@ -0,0 +1,117 @@ +// RUN: %check_clang_tidy %s misc-suspicious-semicolon %t + +int x = 5; + +void nop(); + +void correct1() +{ + if(x < 5) nop(); +} + +void correct2() +{ + if(x == 5) + nop(); +} + +void correct3() +{ + if(x > 5) + { + nop(); + } +} + +void fail1() +{ + if(x > 5); nop(); + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: potentially unintended semicolon [misc-suspicious-semicolon] + // CHECK-FIXES: if(x > 5) nop(); +} + +void fail2() +{ + if(x == 5); + nop(); + // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: potentially unintended semicolon [misc-suspicious-semicolon] + // CHECK-FIXES: if(x == 5){{$}} +} + +void fail3() +{ + if(x < 5); + { + nop(); + } + // CHECK-MESSAGES: :[[@LINE-4]]:11: warning: potentially unintended semicolon + // CHECK-FIXES: if(x < 5){{$}} +} + +void correct4() +{ + while(x % 5 == 1); + nop(); +} + +void correct5() +{ + for(int i = 0; i < x; ++i) + ; +} + +void fail4() +{ + for(int i = 0; i < x; ++i); + nop(); + // CHECK-MESSAGES: :[[@LINE-2]]:28: warning: potentially unintended semicolon + // CHECK-FIXES: for(int i = 0; i < x; ++i){{$}} +} + +void fail5() +{ + if(x % 5 == 1); + nop(); + // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: potentially unintended semicolon + // CHECK-FIXES: if(x % 5 == 1){{$}} +} + +void fail6() { + int a = 0; + if (a != 0) { + } else if (a != 1); +a = 2; + // CHECK-MESSAGES: :[[@LINE-2]]:21: warning: potentially unintended semicolon + // CHECK-FIXES: } else if (a != 1){{$}} +} + +void fail7() { + if (true) +; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: potentially unintended semicolon +} + +void correct6() +{ + do; while(false); +} + +int correct7() +{ + int t_num = 0; + char c = 'b'; + char *s = "a"; + if (s == "(" || s != "'" || c == '"') { +t_num += 3; +return (c == ')' && c == '\''); + } + + return 0; +} + +void correct8() { + if (true) +; + else { + } +} Index: docs/clang-tidy/checks/misc-suspicious-semicolon.rst === --- /dev/null +++ docs/clang-tidy/checks/misc-suspicious-semicolon.rst @@ -0,0 +1,72 @@ +.. title:: clang-tidy - misc-suspicious-semicolon + +misc-suspicious-semicolon += + +Finds most instances of stray semicolons that unexpectedly alter the meaning of +the code. More specifically, it looks for `if`, `while`, `for` and `for-range` +statements whose body is a single semicolon, and then analyzes the context of +the code (e.g. indentation) in an attempt to determine whether that is +intentional. + + .. code-block:: c++ + +if(x < y); +{ + x++; +} + +Here the body of the `if` statement consists of only the semicolon at the end of +the first line, and `x` will be incremented regardless of the condition. + + + .. code-block:: c++ + +while((line = readLine(file)) != NULL); + processLine(line); + +As a result of this code, `processLine()` will only be called once, when the +`while` loop with the empty body exits with `line == NULL`. The indentation of +the code indicates the intention of the programmer. + + + .. code-block:: c++ + +if(x >= y); +x -= y; + +While the indentation does not imply any nesting, there is simply no valid +reason to have an `if` statement with an empty body (but it can make sense for +a loop). + +To solve the issue remove the stray semicolon or in case the empty body is +intentional, reflect this using code indentation or put the semicolon in a new +line. For example: + + .. code-block:: c++ + +while(readWhitespace()); + Token t = readNextToken(); + +Here the second line is indented in a way that suggests that it is meant to be +the body of the `while` loop - whose body is in fact empty, becasue of the +semicolon at the end of the first line. + +Either remove the indentation from the second line: + + .. code-block:: c++ + +while(readWhitespace()); +Token t = readNextToken(); + +... or move the semicolon from the end of the first line to a new line: + + .. code-block:: c++ + +while(readWhitespace()) + ; + + Token t = readNextToken(); + +In this case the check will assume that you know what you are doing, and will +not raise a warning. Index: docs/clang-tidy/checks/list.rst === --- docs/clang-tidy/checks/
Re: [PATCH] D16535: [clang-tidy] Check to find unintended semicolons that changes the semantics.
xazax.hun added a comment. Thank you, good catches! http://reviews.llvm.org/D16535 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16535: [clang-tidy] Check to find unintended semicolons that changes the semantics.
hokein added inline comments. Comment at: docs/clang-tidy/checks/misc-suspicious-semicolon.rst:35 @@ +34,3 @@ + +if(x >= y); +x -= y; The doc needs to be updated. With your latest patch, this is also a warning case. http://reviews.llvm.org/D16535 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16535: [clang-tidy] Check to find unintended semicolons that changes the semantics.
xazax.hun added inline comments. Comment at: docs/clang-tidy/checks/misc-suspicious-semicolon.rst:35 @@ +34,3 @@ + +if(x >= y); +x -= y; hokein wrote: > The doc needs to be updated. > > With your latest patch, this is also a warning case. The documentation implies that this is a warning case but I will make it clearer to be less confusing. http://reviews.llvm.org/D16535 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16535: [clang-tidy] Check to find unintended semicolons that changes the semantics.
xazax.hun updated this revision to Diff 47446. xazax.hun added a comment. - Documentation clarification. http://reviews.llvm.org/D16535 Files: clang-tidy/misc/CMakeLists.txt clang-tidy/misc/MiscTidyModule.cpp clang-tidy/misc/SuspiciousSemicolonCheck.cpp clang-tidy/misc/SuspiciousSemicolonCheck.h docs/clang-tidy/checks/list.rst docs/clang-tidy/checks/misc-suspicious-semicolon.rst test/clang-tidy/misc-suspicious-semicolon.cpp Index: test/clang-tidy/misc-suspicious-semicolon.cpp === --- /dev/null +++ test/clang-tidy/misc-suspicious-semicolon.cpp @@ -0,0 +1,117 @@ +// RUN: %check_clang_tidy %s misc-suspicious-semicolon %t + +int x = 5; + +void nop(); + +void correct1() +{ + if(x < 5) nop(); +} + +void correct2() +{ + if(x == 5) + nop(); +} + +void correct3() +{ + if(x > 5) + { + nop(); + } +} + +void fail1() +{ + if(x > 5); nop(); + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: potentially unintended semicolon [misc-suspicious-semicolon] + // CHECK-FIXES: if(x > 5) nop(); +} + +void fail2() +{ + if(x == 5); + nop(); + // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: potentially unintended semicolon [misc-suspicious-semicolon] + // CHECK-FIXES: if(x == 5){{$}} +} + +void fail3() +{ + if(x < 5); + { + nop(); + } + // CHECK-MESSAGES: :[[@LINE-4]]:11: warning: potentially unintended semicolon + // CHECK-FIXES: if(x < 5){{$}} +} + +void correct4() +{ + while(x % 5 == 1); + nop(); +} + +void correct5() +{ + for(int i = 0; i < x; ++i) + ; +} + +void fail4() +{ + for(int i = 0; i < x; ++i); + nop(); + // CHECK-MESSAGES: :[[@LINE-2]]:28: warning: potentially unintended semicolon + // CHECK-FIXES: for(int i = 0; i < x; ++i){{$}} +} + +void fail5() +{ + if(x % 5 == 1); + nop(); + // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: potentially unintended semicolon + // CHECK-FIXES: if(x % 5 == 1){{$}} +} + +void fail6() { + int a = 0; + if (a != 0) { + } else if (a != 1); +a = 2; + // CHECK-MESSAGES: :[[@LINE-2]]:21: warning: potentially unintended semicolon + // CHECK-FIXES: } else if (a != 1){{$}} +} + +void fail7() { + if (true) +; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: potentially unintended semicolon +} + +void correct6() +{ + do; while(false); +} + +int correct7() +{ + int t_num = 0; + char c = 'b'; + char *s = "a"; + if (s == "(" || s != "'" || c == '"') { +t_num += 3; +return (c == ')' && c == '\''); + } + + return 0; +} + +void correct8() { + if (true) +; + else { + } +} Index: docs/clang-tidy/checks/misc-suspicious-semicolon.rst === --- /dev/null +++ docs/clang-tidy/checks/misc-suspicious-semicolon.rst @@ -0,0 +1,72 @@ +.. title:: clang-tidy - misc-suspicious-semicolon + +misc-suspicious-semicolon += + +Finds most instances of stray semicolons that unexpectedly alter the meaning of +the code. More specifically, it looks for `if`, `while`, `for` and `for-range` +statements whose body is a single semicolon, and then analyzes the context of +the code (e.g. indentation) in an attempt to determine whether that is +intentional. + + .. code-block:: c++ + +if(x < y); +{ + x++; +} + +Here the body of the `if` statement consists of only the semicolon at the end of +the first line, and `x` will be incremented regardless of the condition. + + + .. code-block:: c++ + +while((line = readLine(file)) != NULL); + processLine(line); + +As a result of this code, `processLine()` will only be called once, when the +`while` loop with the empty body exits with `line == NULL`. The indentation of +the code indicates the intention of the programmer. + + + .. code-block:: c++ + +if(x >= y); +x -= y; + +While the indentation does not imply any nesting, there is simply no valid +reason to have an `if` statement with an empty body (but it can make sense for +a loop). So this check issues a warning for the code above. + +To solve the issue remove the stray semicolon or in case the empty body is +intentional, reflect this using code indentation or put the semicolon in a new +line. For example: + + .. code-block:: c++ + +while(readWhitespace()); + Token t = readNextToken(); + +Here the second line is indented in a way that suggests that it is meant to be +the body of the `while` loop - whose body is in fact empty, becasue of the +semicolon at the end of the first line. + +Either remove the indentation from the second line: + + .. code-block:: c++ + +while(readWhitespace()); +Token t = readNextToken(); + +... or move the semicolon from the end of the first line to a new line: + + .. code-block:: c++ + +while(readWhitespace()) + ; + + Token t = readNextToken(); + +In this case the check will assume that you know what you are doing, and will +not raise a warning. Index: docs/clang-tidy/checks/list.rst ==
Re: [PATCH] D16535: [clang-tidy] Check to find unintended semicolons that changes the semantics.
hokein added a comment. LGTM, thanks for working on this! Ping @alexfh http://reviews.llvm.org/D16535 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16535: [clang-tidy] Check to find unintended semicolons that changes the semantics.
alexfh accepted this revision. alexfh added a comment. LG. Thanks! Could you run the check on LLVM and post here a summary of results: how many warnings are generated, whether there are any false positives (based on a reasonably-sized random sample, if there are too many warnings to inspect all relevant code), etc. http://reviews.llvm.org/D16535 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16535: [clang-tidy] Check to find unintended semicolons that changes the semantics.
This revision was automatically updated to reflect the committed changes. Closed by commit rL260503: [clang-tidy] Add a check to find unintended semicolons that changes the… (authored by xazax). Changed prior to commit: http://reviews.llvm.org/D16535?vs=47446&id=47604#toc Repository: rL LLVM http://reviews.llvm.org/D16535 Files: clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp clang-tools-extra/trunk/clang-tidy/misc/SuspiciousSemicolonCheck.cpp clang-tools-extra/trunk/clang-tidy/misc/SuspiciousSemicolonCheck.h clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst clang-tools-extra/trunk/docs/clang-tidy/checks/misc-suspicious-semicolon.rst clang-tools-extra/trunk/test/clang-tidy/misc-suspicious-semicolon.cpp Index: clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt === --- clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt +++ clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt @@ -21,6 +21,7 @@ SizeofContainerCheck.cpp StaticAssertCheck.cpp StringIntegerAssignmentCheck.cpp + SuspiciousSemicolonCheck.cpp SwappedArgumentsCheck.cpp ThrowByValueCatchByReferenceCheck.cpp UndelegatedConstructor.cpp Index: clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp === --- clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp +++ clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp @@ -29,6 +29,7 @@ #include "SizeofContainerCheck.h" #include "StaticAssertCheck.h" #include "StringIntegerAssignmentCheck.h" +#include "SuspiciousSemicolonCheck.h" #include "SwappedArgumentsCheck.h" #include "ThrowByValueCatchByReferenceCheck.h" #include "UndelegatedConstructor.h" @@ -81,6 +82,8 @@ "misc-static-assert"); CheckFactories.registerCheck( "misc-string-integer-assignment"); +CheckFactories.registerCheck( +"misc-suspicious-semicolon"); CheckFactories.registerCheck( "misc-swapped-arguments"); CheckFactories.registerCheck( Index: clang-tools-extra/trunk/clang-tidy/misc/SuspiciousSemicolonCheck.cpp === --- clang-tools-extra/trunk/clang-tidy/misc/SuspiciousSemicolonCheck.cpp +++ clang-tools-extra/trunk/clang-tidy/misc/SuspiciousSemicolonCheck.cpp @@ -0,0 +1,74 @@ +//===--- SuspiciousSemicolonCheck.cpp - clang-tidy-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===--===// + +#include "../utils/LexerUtils.h" +#include "SuspiciousSemicolonCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace misc { + +void SuspiciousSemicolonCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + stmt(anyOf(ifStmt(hasThen(nullStmt().bind("semi")), +unless(hasElse(stmt(, + forStmt(hasBody(nullStmt().bind("semi"))), + cxxForRangeStmt(hasBody(nullStmt().bind("semi"))), + whileStmt(hasBody(nullStmt().bind("semi") + .bind("stmt"), + this); +} + +void SuspiciousSemicolonCheck::check(const MatchFinder::MatchResult &Result) { + const auto *Semicolon = Result.Nodes.getNodeAs("semi"); + SourceLocation LocStart = Semicolon->getLocStart(); + + if (LocStart.isMacroID()) +return; + + ASTContext &Ctxt = *Result.Context; + auto Token = lexer_utils::getPreviousNonCommentToken(Ctxt, LocStart); + auto &SM = *Result.SourceManager; + unsigned SemicolonLine = SM.getSpellingLineNumber(LocStart); + + const auto *Statement = Result.Nodes.getNodeAs("stmt"); + const bool IsIfStmt = isa(Statement); + + if (!IsIfStmt && + SM.getSpellingLineNumber(Token.getLocation()) != SemicolonLine) +return; + + SourceLocation LocEnd = Semicolon->getLocEnd(); + FileID FID = SM.getFileID(LocEnd); + llvm::MemoryBuffer *Buffer = SM.getBuffer(FID, LocEnd); + Lexer Lexer(SM.getLocForStartOfFile(FID), Ctxt.getLangOpts(), + Buffer->getBufferStart(), SM.getCharacterData(LocEnd) + 1, + Buffer->getBufferEnd()); + if (Lexer.LexFromRawLexer(Token)) +return; + + unsigned BaseIndent = SM.getSpellingColumnNumber(Statement->getLocStart()); + unsigned NewTokenIndent = SM.getSpellingColumnNumber(Token.getLocation()); + unsigned NewTokenLine = SM.getSpellingLineNumber(Token.getLocation()); + + if (!IsIfStmt && NewTokenIndent <= BaseIndent && + Token.getKind() != tok::l_brace && NewTokenLine != SemicolonLine) +return; + + diag(LocStart, "potentially unintended semicolon") + << FixItHint::C
Re: [PATCH] D16535: [clang-tidy] Check to find unintended semicolons that changes the semantics.
xazax.hun added a comment. In http://reviews.llvm.org/D16535#348469, @alexfh wrote: > Could you run the check on LLVM and post here a summary of results: how many > warnings are generated, whether there are any false positives (based on a > reasonably-sized random sample, if there are too many warnings to inspect all > relevant code), etc. This check did not give any results for LLVM codebase. I did not see any false positive on other projects yet, so I assume the false positive rate should be low (but might depend on coding style). Repository: rL LLVM http://reviews.llvm.org/D16535 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16535: [clang-tidy] Check to find unintended semicolons that changes the semantics.
omtcyf0 added a subscriber: omtcyf0. omtcyf0 added a comment. @xazax.hun did you actually run the tool on the LLVM codebase? Because this check generates **tons** of false-positive reports during codebase analysis. See the minimal example below. omtcyf0-laptop:playground omtcyf0$ cat main.cpp #include int main() { std::vector numbers = {1, 2, 3, 4, 5, 6}; for (std::vector::const_iterator it = begin(numbers), end = end(numbers); it != end; ++it) { (*it)++; } return 0; } omtcyf0-laptop:playground omtcyf0$ /Users/omtcyf0/Documents/dev/build/Release/llvm/bin/clang-tidy -checks=misc-suspicious-semicolon main.cpp Error while trying to load a compilation database: Could not auto-detect compilation database for file "main.cpp" No compilation database found in /Users/omtcyf0/Documents/dev/playground or any parent directory json-compilation-database: Error while opening JSON database: No such file or directory Running without flags. 1 warning and 1 error generated. Error while processing /Users/omtcyf0/Documents/dev/playground/main.cpp. /Users/omtcyf0/Documents/dev/playground/main.cpp:6:17: warning: potentially unintended semicolon [misc-suspicious-semicolon] it != end; ++it) { ^ /usr/include/wchar.h:89:10: error: 'stdarg.h' file not found [clang-diagnostic-error] #include ^ And this is happening all over the LLVM codebase, because there is nothing bad there. Can you please fix that? Repository: rL LLVM http://reviews.llvm.org/D16535 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16535: [clang-tidy] Check to find unintended semicolons that changes the semantics.
alexfh added a comment. In http://reviews.llvm.org/D16535#362685, @omtcyf0 wrote: > @xazax.hun did you actually run the tool on the LLVM codebase? > > Because this check generates **tons** of false-positive reports during > codebase analysis. > > See the minimal example below. > > omtcyf0-laptop:playground omtcyf0$ cat main.cpp > #include > > int main() { > std::vector numbers = {1, 2, 3, 4, 5, 6}; > for (std::vector::iterator it = std::begin(numbers), > end = std::end(numbers); > it != end; ++it) { > (*it)++; > } > return 0; > } > omtcyf0-laptop:playground omtcyf0$ > /Users/omtcyf0/Documents/dev/build/Release/llvm/bin/clang-tidy > -checks=misc-suspicious-semicolon main.cpp > Error while trying to load a compilation database: > Could not auto-detect compilation database for file "main.cpp" > No compilation database found in /Users/omtcyf0/Documents/dev/playground or > any parent directory > json-compilation-database: Error while opening JSON database: No such file > or directory > Running without flags. > 1 warning and 1 error generated. > Error while processing /Users/omtcyf0/Documents/dev/playground/main.cpp. > /Users/omtcyf0/Documents/dev/playground/main.cpp:6:17: warning: potentially > unintended semicolon [misc-suspicious-semicolon] > it != end; ++it) { > ^ > /usr/include/wchar.h:89:10: error: 'stdarg.h' file not found > [clang-diagnostic-error] > #include >^ > > > And this is happening all over the LLVM codebase, because there is nothing > bad there. > > Can you please fix that? Kirill, the problem in your case may be related to the check seeing incomplete AST due to compilation errors. Can you append `-- -std=c++11` to your clang-tidy invocation and try again whether it will be able to parse the file completely (i.e. without any "file not found" and other compilation errors)? Repository: rL LLVM http://reviews.llvm.org/D16535 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16535: [clang-tidy] Check to find unintended semicolons that changes the semantics.
alexfh added a comment. Having said that, it makes sense to handle invalid AST in the check somehow (e.g. completely disable the check), so it doesn't generate spurious errors. Repository: rL LLVM http://reviews.llvm.org/D16535 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16535: [clang-tidy] Check to find unintended semicolons that changes the semantics.
omtcyf0 added a comment. In http://reviews.llvm.org/D16535#362689, @alexfh wrote: > In http://reviews.llvm.org/D16535#362685, @omtcyf0 wrote: > > > @xazax.hun did you actually run the tool on the LLVM codebase? > > > > Because this check generates **tons** of false-positive reports during > > codebase analysis. > > > > See the minimal example below. > > > > omtcyf0-laptop:playground omtcyf0$ cat main.cpp > > #include > > > > int main() { > > std::vector numbers = {1, 2, 3, 4, 5, 6}; > > for (std::vector::iterator it = std::begin(numbers), > > end = std::end(numbers); > > it != end; ++it) { > > (*it)++; > > } > > return 0; > > } > > omtcyf0-laptop:playground omtcyf0$ > > /Users/omtcyf0/Documents/dev/build/Release/llvm/bin/clang-tidy > > -checks=misc-suspicious-semicolon main.cpp > > Error while trying to load a compilation database: > > Could not auto-detect compilation database for file "main.cpp" > > No compilation database found in /Users/omtcyf0/Documents/dev/playground > > or any parent directory > > json-compilation-database: Error while opening JSON database: No such > > file or directory > > Running without flags. > > 1 warning and 1 error generated. > > Error while processing /Users/omtcyf0/Documents/dev/playground/main.cpp. > > /Users/omtcyf0/Documents/dev/playground/main.cpp:6:17: warning: > > potentially unintended semicolon [misc-suspicious-semicolon] > > it != end; ++it) { > > ^ > > /usr/include/wchar.h:89:10: error: 'stdarg.h' file not found > > [clang-diagnostic-error] > > #include > >^ > > > > > > And this is happening all over the LLVM codebase, because there is nothing > > bad there. > > > > Can you please fix that? > > > Kirill, the problem in your case may be related to the check seeing > incomplete AST due to compilation errors. Can you append `-- -std=c++11` to > your clang-tidy invocation and try again whether it will be able to parse the > file completely (i.e. without any "file not found" and other compilation > errors)? Yes, you're right, it does suppress the warning. However, running vanilla `run-clang-tidy.py` on the LLVM codebase still outputs such issues. Is it the `run-clang-tidy.py`'s fault? The compilation database contains the -std=c++11 specifiers, I assumed this gets added to the clang-tidy options while performing analysis of these sources, doesn't it? Repository: rL LLVM http://reviews.llvm.org/D16535 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16535: [clang-tidy] Check to find unintended semicolons that changes the semantics.
omtcyf0 added a comment. Hm. Seems like this **is** either me using `run-clang-tidy.py` wrong or it working not properly, because I still get compilation errors. I believe I've done the same thing yesterday using the older tree and I didn't get any. Thus said, everything's alright if the AST is acquired correctly. Otherwise the check works not as expected, so the fix Alexander wrote about is needed. Repository: rL LLVM http://reviews.llvm.org/D16535 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16535: [clang-tidy] Check to find unintended semicolons that changes the semantics.
alexfh added a comment. I think, the check can be submitted as is and guards against spurious errors on invalid AST can be added as a follow up. Repository: rL LLVM http://reviews.llvm.org/D16535 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16535: [clang-tidy] Check to find unintended semicolons that changes the semantics.
xazax.hun added a comment. In http://reviews.llvm.org/D16535#362726, @alexfh wrote: > I think, the check can be submitted as is and guards against spurious errors > on invalid AST can be added as a follow up. This check is already submitted. However I did not found any API in tidy to check whether the compilation failed. It would be great to be able to query that information at the time of registering matchers. I have one more question though: does it make sense to run clang tidy at all, when the compilation failed? Isn't it a better default behaviour to not to run any of the checks in that case? Repository: rL LLVM http://reviews.llvm.org/D16535 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16535: [clang-tidy] Check to find unintended semicolons that changes the semantics.
alexfh added a comment. In http://reviews.llvm.org/D16535#362761, @xazax.hun wrote: > In http://reviews.llvm.org/D16535#362726, @alexfh wrote: > > > I think, the check can be submitted as is and guards against spurious > > errors on invalid AST can be added as a follow up. > > > This check is already submitted. Yup, /me needs to sleep more ;) > However I did not found any API in tidy to check whether the compilation > failed. It would be great to be able to query that information at the time of > registering matchers. This information is not available when registering matchers, since this happens before parsing. You can use `isInvalidDecl()` to check whether particular declaration had any errors, or `Result.Context->getTranslationUnitDecl()->isInvalidDecl()` to check the whole translation unit. > I have one more question though: does it make sense to run clang tidy at all, > when the compilation failed? Isn't it a better default behaviour to not to > run any of the checks in that case? There are some valid use cases for running checks on a non-compilable code, e.g. editor integration. Repository: rL LLVM http://reviews.llvm.org/D16535 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16535: [clang-tidy] Check to find unintended semicolons that changes the semantics.
LegalizeAdulthood added a subscriber: LegalizeAdulthood. Comment at: docs/clang-tidy/checks/misc-suspicious-semicolon.rst:48-49 @@ +47,4 @@ + +while(readWhitespace()); + Token t = readNextToken(); + In this documentation, can we please format code so that control structures don't look like function calls? By this I mean write: ``` if (x >=y); x -= y; while (readwhitespace()); Token t = readNextToken(); ``` instead of ``` if(x >=y); x -= y; while(readwhitespace()); Token t = readNextToken(); ``` Repository: rL LLVM http://reviews.llvm.org/D16535 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16535: [clang-tidy] Check to find unintended semicolons that changes the semantics.
xazax.hun added a comment. The comments should be addressed in: http://reviews.llvm.org/rL262615 Repository: rL LLVM http://reviews.llvm.org/D16535 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16535: [clang-tidy] Check to find unintended semicolons that changes the semantics.
kimgr added a subscriber: kimgr. kimgr added a comment. Cool check, I like how it pays attention to indentation! I found some minor doc nits, see inline. Comment at: docs/clang-tidy/checks/misc-suspicious-semicolon.rst:8-9 @@ +7,4 @@ +the code. More specifically, it looks for `if`, `while`, `for` and `for-range` +statements whose body is a single semicolon, and then analyzes the +context of the code (e.g. indentation) in an attempt to determine whether that +is intentional. "context" looks like it would fit on the wrapped line. Comment at: docs/clang-tidy/checks/misc-suspicious-semicolon.rst:20 @@ +19,3 @@ +Here the body of the `if` statement consists of only the semicolon at the end of +the first line, and `x` will be increased regardless of the condition. + incremented? Comment at: docs/clang-tidy/checks/misc-suspicious-semicolon.rst:29 @@ +28,3 @@ +As a result of this code, `processLine()` will only be called once, when the +`while` loop with the empty body exits with `line == NULL`. The identation of +the code indicates the intention of the programmer. Typo: identation Comment at: docs/clang-tidy/checks/misc-suspicious-semicolon.rst:71 @@ +70,3 @@ + +In this case the checker will assume that you know what you are doing, and will +not raise a warning. There's been some preference for "check" over "checker" lately. Comment at: test/clang-tidy/misc-suspicious-semicolon.cpp:88 @@ +87,3 @@ + char c = 'b'; + char * s = "a"; + if (s == "(" || s != "'" || c == '"') { Weird pointer alignment is a little distracting here, better stick with LLVM convention and attach it to `s`? http://reviews.llvm.org/D16535 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16535: [clang-tidy] Check to find unintended semicolons that changes the semantics.
xazax.hun marked 5 inline comments as done. xazax.hun added a comment. http://reviews.llvm.org/D16535 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16535: [clang-tidy] Check to find unintended semicolons that changes the semantics.
xazax.hun updated this revision to Diff 45949. xazax.hun added a comment. - Fixed the nits pointed out by the review. http://reviews.llvm.org/D16535 Files: clang-tidy/misc/CMakeLists.txt clang-tidy/misc/MiscTidyModule.cpp clang-tidy/misc/SuspiciousSemicolonCheck.cpp clang-tidy/misc/SuspiciousSemicolonCheck.h docs/clang-tidy/checks/list.rst docs/clang-tidy/checks/misc-suspicious-semicolon.rst test/clang-tidy/misc-suspicious-semicolon.cpp Index: test/clang-tidy/misc-suspicious-semicolon.cpp === --- /dev/null +++ test/clang-tidy/misc-suspicious-semicolon.cpp @@ -0,0 +1,95 @@ +// RUN: %check_clang_tidy %s misc-suspicious-semicolon %t + +int x = 5; + +void nop(); + +void correct1() +{ + if(x < 5) nop(); +} + +void correct2() +{ + if(x == 5) + nop(); +} + +void correct3() +{ + if(x > 5) + { + nop(); + } +} + +void fail1() +{ + if(x > 5); nop(); + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: potentially unintended semicolon [misc-suspicious-semicolon] + // CHECK-FIXES: if(x > 5) nop(); +} + +void fail2() +{ + if(x == 5); + nop(); + // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: potentially unintended semicolon [misc-suspicious-semicolon] + // CHECK-FIXES: if(x == 5){{$}} +} + +void fail3() +{ + if(x < 5); + { + nop(); + } + // CHECK-MESSAGES: :[[@LINE-4]]:11: warning: potentially unintended semicolon + // CHECK-FIXES: if(x < 5){{$}} +} + +void correct4() +{ + while(x % 5 == 1); + nop(); +} + +void correct5() +{ + for(int i = 0; i < x; ++i) + ; +} + +void fail4() +{ + for(int i = 0; i < x; ++i); + nop(); + // CHECK-MESSAGES: :[[@LINE-2]]:28: warning: potentially unintended semicolon + // CHECK-FIXES: for(int i = 0; i < x; ++i){{$}} +} + +void fail5() +{ + if(x % 5 == 1); + nop(); + // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: potentially unintended semicolon + // CHECK-FIXES: if(x % 5 == 1){{$}} +} + +void correct6() +{ + do; while(false); +} + +int correct7() +{ + int t_num = 0; + char c = 'b'; + char *s = "a"; + if (s == "(" || s != "'" || c == '"') { +t_num += 3; +return (c == ')' && c == '\''); + } + + return 0; +} Index: docs/clang-tidy/checks/misc-suspicious-semicolon.rst === --- /dev/null +++ docs/clang-tidy/checks/misc-suspicious-semicolon.rst @@ -0,0 +1,72 @@ +.. title:: clang-tidy - misc-suspicious-semicolon + +misc-suspicious-semicolon += + +Finds most instances of stray semicolons that unexpectedly alter the meaning of +the code. More specifically, it looks for `if`, `while`, `for` and `for-range` +statements whose body is a single semicolon, and then analyzes the context of +the code (e.g. indentation) in an attempt to determine whether that is +intentional. + + .. code-block:: c++ + +if(x < y); +{ + x++; +} + +Here the body of the `if` statement consists of only the semicolon at the end of +the first line, and `x` will be incremented regardless of the condition. + + + .. code-block:: c++ + +while((line = readLine(file)) != NULL); + processLine(line); + +As a result of this code, `processLine()` will only be called once, when the +`while` loop with the empty body exits with `line == NULL`. The indentation of +the code indicates the intention of the programmer. + + + .. code-block:: c++ + +if(x >= y); +x -= y; + +While the indentation does not imply any nesting, there is simply no valid +reason to have an `if` statement with an empty body (but it can make sense for +a loop). + +To solve the issue remove the stray semicolon or in case the empty body is +intentional, reflect this using code indentation or put the semicolon in a new +line. For example: + + .. code-block:: c++ + +while(readWhitespace()); + Token t = readNextToken(); + +Here the second line is indented in a way that suggests that it is meant to be +the body of the `while` loop - whose body is in fact empty, becasue of the +semicolon at the end of the first line. + +Either remove the indentation from the second line: + + .. code-block:: c++ + +while(readWhitespace()); +Token t = readNextToken(); + +... or move the semicolon from the end of the first line to a new line: + + .. code-block:: c++ + +while(readWhitespace()) + ; + + Token t = readNextToken(); + +In this case the check will assume that you know what you are doing, and will +not raise a warning. Index: docs/clang-tidy/checks/list.rst === --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -58,6 +58,7 @@ misc-sizeof-container misc-static-assert misc-string-integer-assignment + misc-suspicious-semicolon misc-swapped-arguments misc-throw-by-value-catch-by-reference misc-undelegated-constructor Index: clang-tidy/misc/SuspiciousSemicolonCheck.h ===
Re: [PATCH] D16535: [clang-tidy] Check to find unintended semicolons that changes the semantics.
alexfh added a comment. Sorry for the long delay. Currently rather swamped. Haojian, could you take a look at this patch? http://reviews.llvm.org/D16535 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits