Page MenuHomePhabricator

[clang-tidy] Add a utility Matcher to match the next statement within a statement sequence
AbandonedPublic

Authored by tbourvon on Jan 28 2018, 10:54 AM.

Details

Summary

This adds a utility matcher (which is placed in util/Matchers.h, because it uses functions from ASTMatchFinder.h which cannot be used from ASTMatchers.h) used by D37014).
This matcher matches the next statement in a statement sequence (such as CompoundStatement). It handles switch statements gracefully.

Diff Detail

Event Timeline

tbourvon created this revision.Jan 28 2018, 10:54 AM
lebedev.ri edited reviewers, added: alexfh, aaron.ballman; removed: lebedev.ri.Jan 28 2018, 11:00 AM
lebedev.ri set the repository for this revision to rCTE Clang Tools Extra.
aaron.ballman added inline comments.Jan 29 2018, 5:27 AM
clang-tidy/utils/Matchers.h
16

This will require linking in the clangAnalysis library as well; are we sure we want to take on this dependency here for all matchers?

58–60

Why does it cause the crash? Should that crash not be fixed instead of applying this workaround?

70

No need for the parenthetical. It's generally understood what a CFG is, so you can just use the acronym.

74–75

Elide braces (here and elsewhere).

81

Formatting (here and elsewhere): you should run the patch through clang-format.

tbourvon added inline comments.Feb 13 2018, 6:25 AM
clang-tidy/utils/Matchers.h
16

Do you have a specific solution in mind? We could make the matcher local to the check it is being used in (see D37014), but I think it could prove useful for other checks...

58–60

I'm not entirely sure if this is expected behavior or not. In terms of AST, switch statements are a bit special in terms of how they are represented (each case contains all the subsequent cases as its children IIRC).
There probably is a way to make the CFG work in these cases, but I honestly don't have the time to look into that and attempt a fix. Couldn't this be good enough for now, maybe with a FIXME?

aaron.ballman added inline comments.Feb 13 2018, 10:18 AM
clang-tidy/utils/Matchers.h
16

No solution in mind -- I was mostly wondering if @alexfh was okay picking up this dependency or not, because it will impact all the clang-tidy modules.

58–60

I'm uncomfortable simply working around known crashes rather than fixing them, even with FIXME comments, because that just means we accrue more technical debt with no plan in place to pay it down in the future. However, I'll let @alexfh make the final call on it.

@alexfh What is your opinion regarding adding this dependency?

alexfh added a comment.Mar 6 2018, 3:16 AM

@alexfh What is your opinion regarding adding this dependency?

I'm fine with the dependency, and I'd probably make the matcher local to the check until there is at least one more use case for it in the codebase.

@aaron.ballman Hello, Do you think it is ready to land? Thanks

@aaron.ballman Hello, Do you think it is ready to land? Thanks

@alexfh was asking to make this matcher local to the check rather than add it to Matchers.h, so I think this patch should be abandoned and rolled into D37014 directly, but the dependency is fine to pick up in that patch.

tbourvon abandoned this revision.Mar 8 2018, 8:56 AM