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
