[PATCH] D19586: Misleading Indentation check

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added inline comments.


> MisleadingIndentationCheck.cpp:20
> +
> +void MisleadingIndentationCheck::danglingElseCheck(
> +    const MatchFinder::MatchResult &Result) {

There is no handling of tabs and spaces by danglingElseCheck as far as I see.

The "if" might for example be indented with spaces. And then the corresponding "else" is indented with a tab. Then I guess there is false positive.

If the "if" is indented with 2 spaces and the "else" is indented with 2 tabs. then I assume there is false negative.

It's unfortunate. Is it ok?

> MisleadingIndentationCheck.cpp:34
> +      Result.SourceManager->getExpansionColumnNumber(ElseLoc))
> +    diag(ElseLoc, "potential dangling else");
> +}

I see no test case that says "potential dangling else". If I'm not mistaken that should be added.

Is it really sufficient to write that? It's not obvious to me why clang-tidy would think it's dangling.

> MisleadingIndentationCheck.cpp:44
> +
> +    if (isa<IfStmt>(CurrentStmt)) {
> +      IfStmt *CurrentIf = dyn_cast<IfStmt>(CurrentStmt);

You don't need to use both isa and dyn_cast:

  if (const auto *CurrentIf = dyn_cast<IfStmt>(CurrentStmt)) {
      Inside = CurrentIf->getElse() ? CurrentIf->getElse() : CurrentIf->getThen();
  } ....

> MisleadingIndentationCheck.h:20
> +/// Checks the code for dangling else,
> +///       and possible misleading indentations due to missing braces.
> +///

there are too many spaces in this comment

https://reviews.llvm.org/D19586