[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 ↗

(On Diff #91473)

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

[alexfh]

clang-tidy/readability/MisleadingIndentationCheck.cpp

40–42 ↗

(On Diff #91473)

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-tools-extra/trunk/clang-tidy/readability/MisleadingIndentationCheck.h

 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 danglingElseCheck(const SourceManager &SM, ASTContext *Context,
+                         const IfStmt *If);
   void missingBracesCheck(const SourceManager &SM, const CompoundStmt *CStmt);
 };
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_MISLEADING_INDENTATION_H

clang-tools-extra/trunk/clang-tidy/readability/MisleadingIndentationCheck.cpp

 #include "clang/ASTMatchers/ASTMatchFinder.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace readability {
 
+static const IfStmt *getPrecedingIf(const SourceManager &SM,
+                                    ASTContext *Context, const IfStmt *If) {
+  auto parents = Context->getParents(*If);
+  if (parents.size() != 1)
+    return nullptr;
+  if (const auto *PrecedingIf = parents[0].get<IfStmt>()) {
+    SourceLocation PreviousElseLoc = PrecedingIf->getElseLoc();
+    if (SM.getExpansionLineNumber(PreviousElseLoc) ==
+        SM.getExpansionLineNumber(If->getIfLoc()))
+      return PrecedingIf;
+  }
+  return nullptr;
+}
+
 void MisleadingIndentationCheck::danglingElseCheck(const SourceManager &SM,
+                                                   ASTContext *Context,
                                                    const IfStmt *If) {
   SourceLocation IfLoc = If->getIfLoc();
   SourceLocation ElseLoc = If->getElseLoc();
 
   if (IfLoc.isMacroID() || ElseLoc.isMacroID())
     return;
 
   if (SM.getExpansionLineNumber(If->getThen()->getLocEnd()) ==
       SM.getExpansionLineNumber(ElseLoc))
     return;
 
+  // Find location of first 'if' in a 'if else if' chain.
+  for (auto PrecedingIf = getPrecedingIf(SM, Context, If); PrecedingIf;
+       PrecedingIf = getPrecedingIf(SM, Context, PrecedingIf))
+    IfLoc = PrecedingIf->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"};
void MisleadingIndentationCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
       compoundStmt(has(stmt(anyOf(ifStmt(), forStmt(), whileStmt()))))
           .bind("compound"),
       this);
 }
 
 void MisleadingIndentationCheck::check(const MatchFinder::MatchResult &Result) {
   if (const auto *If = Result.Nodes.getNodeAs<IfStmt>("if"))
-    danglingElseCheck(*Result.SourceManager, If);
+    danglingElseCheck(*Result.SourceManager, Result.Context, If);
 
   if (const auto *CStmt = Result.Nodes.getNodeAs<CompoundStmt>("compound"))
     missingBracesCheck(*Result.SourceManager, CStmt);
 }
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang

clang-tools-extra/trunk/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
 }