xazax.hun added inline comments.
Comment at: clang-tidy/readability/MisleadingIndentationCheck.cpp:79
+ Finder->addMatcher(
+ compoundStmt(anyOf(has(ifStmt()), has(forStmt()), has(whileStmt(
+ .bind("compound"),
alexfh wrote:
> xazax.hun
This revision was automatically updated to reflect the committed changes.
Closed by commit rL295041: [clang-tidy] Add readability-misleading-indentation
check. (authored by xazax).
Changed prior to commit:
https://reviews.llvm.org/D19586?vs=88330=88334#toc
Repository:
rL LLVM
alexfh added a comment.
Gábor, thank you for picking up this patch and finishing it!
https://reviews.llvm.org/D19586
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
alexfh accepted this revision.
alexfh added a comment.
This revision is now accepted and ready to land.
LG
Comment at: clang-tidy/readability/MisleadingIndentationCheck.cpp:79
+ Finder->addMatcher(
+ compoundStmt(anyOf(has(ifStmt()), has(forStmt()), has(whileStmt(
+
xazax.hun added a comment.
Thank you for the review!
Comment at: clang-tidy/readability/MisleadingIndentationCheck.cpp:79
+ Finder->addMatcher(
+ compoundStmt(anyOf(has(ifStmt()), has(forStmt()), has(whileStmt(
+ .bind("compound"),
alexfh
xazax.hun updated this revision to Diff 88330.
xazax.hun marked 7 inline comments as done.
xazax.hun added a comment.
- Added a note to make it easier to understand the diagnostics.
- Reworded the error message about dangling else.
- Fixed other review comments.
https://reviews.llvm.org/D19586
danielmarjamaki added inline comments.
Comment at: clang-tidy/readability/MisleadingIndentationCheck.cpp:42
+const Stmt *Inside = nullptr;
+
+if (const auto *CurrentIf = dyn_cast(CurrentStmt)) {
I would rename Inside to Inner. That would make InnerLoc
alexfh requested changes to this revision.
alexfh added inline comments.
This revision now requires changes to proceed.
Comment at: clang-tidy/readability/MisleadingIndentationCheck.cpp:72
+SM.getExpansionColumnNumber(NextLoc))
+ diag(NextLoc, "misleading
xazax.hun marked 13 inline comments as done.
xazax.hun added inline comments.
Comment at: clang-tidy/readability/MisleadingIndentationCheck.cpp:20
+
+void MisleadingIndentationCheck::danglingElseCheck(
+const MatchFinder::MatchResult ) {
danielmarjamaki
xazax.hun updated this revision to Diff 88182.
xazax.hun added a comment.
- Updated to latest trunk.
- Mentioned check in the release notes.
- Documented the limitation that tabs and spaces need to be consistent for this
check to work.
- Fixed (hopefully all) review comments.
- Fixed the test
xazax.hun commandeered this revision.
xazax.hun edited reviewers, added: Pajesz; removed: xazax.hun.
xazax.hun added a comment.
Herald added a subscriber: mgorny.
The original author is no longer available.
https://reviews.llvm.org/D19586
___
danielmarjamaki added inline comments.
> MisleadingIndentationCheck.cpp:20
> +
> +void MisleadingIndentationCheck::danglingElseCheck(
> +const MatchFinder::MatchResult ) {
There is no handling of tabs and spaces by danglingElseCheck as far as I see.
The "if" might for example be indented
Pajesz requested a review of this revision.
Pajesz added reviewers: dkrupp, xazax.hun, nlewycky, etienne.bergeron, etienneb.
Pajesz marked an inline comment as done.
Pajesz added a comment.
Hello!
Gonna fix the tests ASAP! Any other suggestions, fixes, improvements
considering the checker?
xazax.hun added inline comments.
Comment at: test/clang-tidy/readability-misleading-indentation.cpp:16
@@ +15,3 @@
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: wrong indentation,
'else' belongs to 'if(cond2)' statement
+ // CHECK-FIXES: {{^}} //
alexfh added a comment.
Please mark all addressed comments "Done".
Comment at: test/clang-tidy/readability-misleading-indentation.cpp:15
@@ +14,3 @@
+ foo2(); // comment
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: wrong indentation,
'else' belongs to
Pajesz updated this revision to Diff 63925.
Pajesz added a comment.
Minor changes in tests and doc and diff should be full now.
http://reviews.llvm.org/D19586
Files:
clang-tidy/readability/CMakeLists.txt
clang-tidy/readability/MisleadingIndentationCheck.cpp
alexfh added a comment.
Gergely, it seems that the last diff is missing
clang-tidy/readability/MisleadingIndentationCheck.cpp. A few more comments
below.
Comment at: docs/clang-tidy/checks/readability-misleading-indentation.rst:13
@@ +12,3 @@
+
+The way to avoid dangling else
Pajesz updated this revision to Diff 61651.
Pajesz added a comment.
Checker now works with for and while statements as well, new tests were added,
other syntactical and logical updates have been made.
http://reviews.llvm.org/D19586
Files:
clang-tidy/readability/MisleadingIndentationCheck.h
etienneb added a comment.
In http://reviews.llvm.org/D19586#417063, @xazax.hun wrote:
> In http://reviews.llvm.org/D19586#417053, @etienneb wrote:
>
> > The rule also apply for statements in a same compound:
> >
> > {
> > statement1();
> > statement2();
> > statement3();
> >
> >
xazax.hun added a comment.
In http://reviews.llvm.org/D19586#417053, @etienneb wrote:
> The rule also apply for statements in a same compound:
>
> {
> statement1();
> statement2();
> statement3();
>
>
> But this can be a further improvement.
I believe this might be an
etienneb added a comment.
You should not limit the checker to "if".
The for-statement and while-statement are also wrong for the same reasons:
while (x)
statement1();
statement2();
for (...)
statement1();
statement2();
You should test your code over large code base.
You
etienneb requested changes to this revision.
etienneb added a comment.
This revision now requires changes to proceed.
Could you lift some logics to the matcher instead of the check.
Comment at: clang-tidy/readability/MisleadingIndentationCheck.cpp:22
@@ +21,3 @@
+const
Pajesz updated this revision to Diff 55389.
Pajesz marked 5 inline comments as done.
Pajesz added a comment.
Both dangling else and the other check now ignore one-line if-else statements.
Corrected other reviews as well.
http://reviews.llvm.org/D19586
Files:
xazax.hun added a comment.
In http://reviews.llvm.org/D19586#415123, @Pajesz wrote:
> Both dangling else and the other check now ignore one-line if-else
> statements. Corrected other reviews as well.
I can not see a test case for one line if-then-else. Could you add it?
nlewycky added a subscriber: nlewycky.
Comment at: clang-tidy/readability/MisleadingIndentationCheck.cpp:31
@@ +30,3 @@
+ Result.SourceManager->getExpansionColumnNumber(ElseLoc))
+diag(ElseLoc, "potentional dangling else");
+}
Typo of "potential" as
Eugene.Zelenko added a subscriber: Eugene.Zelenko.
Eugene.Zelenko added a comment.
Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).
What about for ans while statements?
Comment at: docs/clang-tidy/checks:10
@@ +9,3 @@
+
+The way to avoid dangling
Pajesz updated this revision to Diff 55247.
Pajesz added a comment.
Probably fixed the broken patch.
http://reviews.llvm.org/D19586
Files:
clang-tidy/readability/CMakeLists.txt
clang-tidy/readability/MisleadingIndentationCheck.cpp
clang-tidy/readability/MisleadingIndentationCheck.h
alexfh added a comment.
The patch is broken:
Index: clang-tidy/readability/Mislead
===
--- /dev/null
+++ clang-tidy/readability/Mislead
@@ -0,0 +1,77 @@
+//===--- MisleadingIndentationCheck.cpp -
Pajesz created this revision.
Pajesz added a reviewer: alexfh.
Pajesz added subscribers: cfe-commits, xazax.hun.
This is a clang-tidy check for llvm. It checks for dangling else and for
possible misleading indentation due to omitted braces.
http://reviews.llvm.org/D19586
Files:
29 matches
Mail list logo
