[PATCH] D19586: Misleading Indentation check

[Etienne Bergeron via cfe-commits]

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