Page MenuHomePhabricator

Removed a RecursiveASTVisitor feature to visit operator kinds with different methods
ClosedPublic

Authored by gribozavr on Jun 30 2020, 4:21 PM.

Details

Summary

This feature was only used in two places, but contributed a non-trivial
amount to the complexity of RecursiveASTVisitor, and was buggy (see my
recent patches where I was fixing the bugs that I noticed). I don't
think the convenience benefit of this feature is worth the complexity.

Besides complexity, another issue with the current state of RecursiveASTVisitor is the non-uniformity in how it handles different AST nodes. All AST nodes follow a regular pattern, but operators are special -- and this special behavior not documented. Correct usage of RecursiveASTVisitor relies on shadowing member functions with specific names and signatures. Near misses don't cause any compile-time errors, incorrectly named or typed methods are just silently ignored. Therefore, predictability of RecursiveASTVisitor API is quite important.

This change reduces the size of the clang binary by 38 KB (0.2%) in
release mode, and by 7 MB (0.3%) in debug mode. The clang-tidy binary
is reduced by 205 KB (0.3%) in release mode, and by 5 MB (0.4%) in debug
mode. I don't think these code size improvements are significant enough
to justify this change on its own (for me, the primary motivation is
reducing code complexity), but they I think are a nice side-effect.

Diff Detail

Event Timeline

gribozavr created this revision.Jun 30 2020, 4:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2020, 4:21 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
gribozavr updated this revision to Diff 274643.Jun 30 2020, 4:23 PM

Removed obsolete comments.

sammccall accepted this revision.Jun 30 2020, 11:56 PM

+1 to the complexity argument, and also for conceptual complexity: there's a lot of value in RAV being regular and predictable given that both its implementation and the AST itself are complex.

I think this should be mentioned in the release notes, as I think it will silently change the behavior of programs that are using these hooks.

This revision is now accepted and ready to land.Jun 30 2020, 11:56 PM
gribozavr2 edited the summary of this revision. (Show Details)Jul 1 2020, 4:11 AM
aaron.ballman accepted this revision.Jul 1 2020, 4:26 AM

LGTM, I think the complexity argument is reasonable (and I appreciate the information showing this also reduces binary size at the same time).

gribozavr2 edited the summary of this revision. (Show Details)Jul 1 2020, 4:36 AM
ymandel accepted this revision.Jul 1 2020, 6:24 AM

+1 Especially given how trivial it was to fix clang/lib/ARCMigrate/TransProperties.cpp, this functionality seems unjustified.

rsmith accepted this revision.Jul 1 2020, 11:58 AM

This introduces a non-uniformity between RecursiveASTVisitor and StmtVisitor. StmtVisitor contains matching logic to treat the various kinds of unary and binary operators as if they were distinct AST node kinds, and that functionality is used much more heavily throughout Clang. Given that, I don't think this change necessarily improves big-picture uniformity, but the reduction in complexity and binary size still seem sufficiently compelling to me.

gribozavr updated this revision to Diff 275064.Jul 2 2020, 4:43 AM

Added release notes.

This revision was automatically updated to reflect the committed changes.