Page MenuHomePhabricator

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

Authored by fgross on Mar 10 2017, 11:49 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

fgross created this revision.Mar 10 2017, 11:49 AM
xazax.hun edited edge metadata.Mar 11 2017, 3:04 AM

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 updated this revision to Diff 91473.Mar 11 2017, 8:16 AM

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 accepted this revision.Mar 11 2017, 1:43 PM

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.

This revision is now accepted and ready to land.Mar 11 2017, 1:43 PM

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

alexfh requested changes to this revision.Mar 16 2017, 7:36 AM

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?

This revision now requires changes to proceed.Mar 16 2017, 7:36 AM
alexfh added inline comments.Mar 16 2017, 7:42 AM
clang-tidy/readability/MisleadingIndentationCheck.cpp
40–42 ↗(On Diff #91473)

ASTContext::getParents is the interface to it.

fgross updated this revision to Diff 92026.Mar 16 2017, 11:17 AM
fgross edited edge metadata.

Now using ASTContext::getParents instead of ChainedIfs map.

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

fgross marked 2 inline comments as done.Mar 16 2017, 11:18 AM
alexfh accepted this revision.Mar 17 2017, 2:50 AM

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.

This revision is now accepted and ready to land.Mar 17 2017, 2:50 AM
This revision was automatically updated to reflect the committed changes.