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 MatchFinder::MatchResult &Result) {
+ auto *If = Result.Nodes.getNodeAs<IfStmt>("if");
+ if (!If->getElse())
----------------
nit: const
================
Comment at: clang-tidy/readability/MisleadingIndentationCheck.cpp:40
@@ +39,3 @@
+ const MatchFinder::MatchResult &Result) {
+ auto *CStmt = Result.Nodes.getNodeAs<CompoundStmt>("stmt");
+ for (unsigned int i = 0; i < CStmt->size() - 1; i++) {
----------------
nit: const
================
Comment at: clang-tidy/readability/MisleadingIndentationCheck.cpp:67
@@ +66,3 @@
+ diag(NextLoc, "Wrong Indentation - statement is indented as a member "
+ "of if statement");
+ }
----------------
of if statement -> previous if statement
================
Comment at: clang-tidy/readability/MisleadingIndentationCheck.cpp:72
@@ +71,3 @@
+void MisleadingIndentationCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(ifStmt().bind("if"), this);
+ Finder->addMatcher(compoundStmt(has(ifStmt())).bind("stmt"), this);
----------------
instead of doing check at line 23:
```
if (!If->getElse())
```
you should use 'hasElse' matcher here.
As I get it, you want to match something like:
```
anyOf(
ifStatement(hasThen(stmt().bind("last")),
hasElse(unless(compoundStmt())),
ifStatement(hasThen(unless(compoundStmt())),
unless(hasElse(stmt().bind("last")))
)
```
Then, by getting node named "last" you can retrieve the indentation of the last
statement.
================
Comment at: clang-tidy/readability/MisleadingIndentationCheck.cpp:77
@@ +76,3 @@
+void MisleadingIndentationCheck::check(const MatchFinder::MatchResult &Result)
{
+
+ if (Result.Nodes.getNodeAs<IfStmt>("if"))
----------------
nit: remove empty line
================
Comment at: clang-tidy/readability/MisleadingIndentationCheck.h:30
@@ +29,3 @@
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ void danglingElseCheck(const ast_matchers::MatchFinder::MatchResult &Result);
+ void missingBracesCheck(const ast_matchers::MatchFinder::MatchResult
&Result);
----------------
nit: private
================
Comment at: docs/clang-tidy/checks/readability-misleading-indentation.rst:8
@@ +7,3 @@
+
+Correct indentation helps to understand code. Mismatch of the syntactical
structure and the indentation of the code may reveal serious pr
+
----------------
could you fix these long lines.
================
Comment at: test/clang-tidy/readability-misleading-indentation.cpp:62
@@ +61,2 @@
+ return 0;
+}
----------------
I would like to see more tests with { }
```
if (xxx) {
} else {
}
```
```
if (xxx) {
}
else {
}
```
```
if (xxx)
{
}
else
{
}
```
```
if (xxx)
{
}
else
{
}
```
================
Comment at: test/clang-tidy/readability-misleading-indentation.cpp:62
@@ +61,2 @@
+ return 0;
+}
----------------
etienneb wrote:
> I would like to see more tests with { }
>
> ```
> if (xxx) {
> } else {
> }
> ```
>
> ```
> if (xxx) {
> }
> else {
> }
> ```
>
> ```
> if (xxx)
> {
> }
> else
> {
> }
> ```
>
> ```
> if (xxx)
> {
> }
> else
> {
> }
> ```
could you add test with macro:
```
#define BLOCK
if (xxx) \
stm1(); \
stm2();
```
http://reviews.llvm.org/D19586
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
