[clang-tidy] readability-misleading-indentation: fix chained if

[fgross]

Summary

Fixed erroneously flagging of chained if statements when styled like this:

if (cond) {
}
else if (cond) {
}
else {
}

[xazax.hun]

Functionally LGTM!

Note that while the traversal of AST Matchers are not defined in general, in this particular case of chained ifs, it is guaranteed that parent nodes will be matched before the child nodes. For this reason I think it is ok to have a state like this. Maybe this worth a comment.

Performance wise maybe an llvm::DenseMap would perform better, according to the LLVM Programmer's Manual, it is a great data structure to map pointers to pointers.

[fgross]

Replaced std::map with llvm::DenseMap, added comment about traversal.

I just assumed it would traverse in the "right" way, is there any documentation about AST / matcher traversal?

[xazax.hun]

In D30841#698634, @fgross wrote:

I just assumed it would traverse in the "right" way, is there any documentation about AST / matcher traversal?

I do not know of any, but I remember having some non-determinism across platforms, but I think matching parent before children is a safe assumption for all platforms.

The patch now looks good to me, but lets wait what @alexfh says.

[fgross]

I don't have commit access, so could someone please commit this for me? Thanks!

[alexfh]

Thank you for the fix! One comment inline.

clang-tidy/readability/MisleadingIndentationCheck.cpp

40–42

Can you just use the parent map to get the previous if in the chain?

[alexfh]

clang-tidy/readability/MisleadingIndentationCheck.cpp

40–42

ASTContext::getParents is the interface to it.

[fgross]

Now using ASTContext::getParents instead of ChainedIfs map.

For some reason I thought of getParents as an expensive function to call...

[alexfh]

In D30841#702980, @fgross wrote:

Now using ASTContext::getParents instead of ChainedIfs map.

For some reason I thought of getParents as an expensive function to call...

Good point. It is expensive first time you call it, since it builds parents map. My idea was that since many checks use hasParent or hasAncestor matchers, the parent map will likely be built anyway, and building a (smaller) map of chained ifs as an optimization won't make much difference. But I don't know for sure, and maybe your approach is indeed better. We can start with the parent-map based one and keep the possible optimization in mind, in case there's an evidence of performance issues.

LG. I'll submit the patch for you.

clang-tidy/readability/MisleadingIndentationCheck.h

 //===--- MisleadingIndentationCheck.h - clang-tidy---------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_MISLEADING_INDENTATION_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_MISLEADING_INDENTATION_H
 
 #include "../ClangTidy.h"
+#include "llvm/ADT/DenseMap.h"
 
 namespace clang {
 namespace tidy {
 namespace readability {
 
 /// Checks the code for dangling else, and possible misleading indentations due
 /// to missing braces. Note that this check only works as expected when the tabs
 /// or spaces are used consistently and not mixed.
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/readability-misleading-indentation.html
 class MisleadingIndentationCheck : public ClangTidyCheck {
 public:
   MisleadingIndentationCheck(StringRef Name, ClangTidyContext *Context)
       : ClangTidyCheck(Name, Context) {}
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:
   void danglingElseCheck(const SourceManager &SM, const IfStmt *If);
   void missingBracesCheck(const SourceManager &SM, const CompoundStmt *CStmt);
+
+  /// Key: Chained If
+  /// Value: Preceding If
+  llvm::DenseMap<const IfStmt *, const IfStmt *> ChainedIfs;
 };
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_MISLEADING_INDENTATION_H

clang-tidy/readability/MisleadingIndentationCheck.cpp

 void MisleadingIndentationCheck::danglingElseCheck(const SourceManager &SM,
                                                    const IfStmt *If) {
   SourceLocation IfLoc = If->getIfLoc();
   SourceLocation ElseLoc = If->getElseLoc();
 
   if (IfLoc.isMacroID() || ElseLoc.isMacroID())
     return;
 
+  if (const auto *ChainedIf = dyn_cast<IfStmt>(If->getElse())) {
+    if (SM.getExpansionLineNumber(ElseLoc) ==
+        SM.getExpansionLineNumber(ChainedIf->getIfLoc()))
+    ChainedIfs.insert({ ChainedIf, If });
+  }
+
   if (SM.getExpansionLineNumber(If->getThen()->getLocEnd()) ==
       SM.getExpansionLineNumber(ElseLoc))
     return;
 
+  // Find location of first 'if' in a 'if else if' chain.
+  // Works because parent nodes will be matched before child nodes.
+  for (auto Iter = ChainedIfs.find(If); Iter != ChainedIfs.end();
+       Iter = ChainedIfs.find(Iter->second))
+    IfLoc = Iter->second->getIfLoc();
+
   if (SM.getExpansionColumnNumber(IfLoc) !=
       SM.getExpansionColumnNumber(ElseLoc))
     diag(ElseLoc, "different indentation for 'if' and corresponding 'else'");
 }
 
 void MisleadingIndentationCheck::missingBracesCheck(const SourceManager &SM,
                                                     const CompoundStmt *CStmt) {
   const static StringRef StmtNames[] = {"if", "for", "while"};

test/clang-tidy/readability-misleading-indentation.cpp

int main()
 
   if (cond1)
     {
     }
   else
     {
     }
 
+  if(cond1) {
+  }
+  else if (cond2) {
+  }
+  else {
+  }
+
+  if(cond1) {
+  }
+  else if (cond2) {
+  }
+       else {
+  }
+  // CHECK-MESSAGES: :[[@LINE-2]]:8: warning: different indentation for 'if' and corresponding 'else' [readability-misleading-indentation]
+  
+  if (cond1) {
+    if (cond1) {
+    }
+    else if (cond2) {
+    }
+    else {
+    }
+  }
+  else if (cond2) {
+  }
+
   BLOCK
 }