[all-commits] [llvm/llvm-project] 734947: RecursiveASTVisitor: don't call WalkUp unnecessari...

[Dmitri Gribenko via All-commits]

  Branch: refs/heads/master
  Home:   https://github.com/llvm/llvm-project
  Commit: 7349479f2244c32c0184ca545af04adb171c8077
      https://github.com/llvm/llvm-project/commit/7349479f2244c32c0184ca545af04adb171c8077
  Author: Dmitri Gribenko <gribozavr at gmail.com>
  Date:   2020-07-06 (Mon, 06 Jul 2020)

  Changed paths:
    M clang/include/clang/AST/RecursiveASTVisitor.h
    M clang/lib/Tooling/Syntax/BuildTree.cpp
    M clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp

  Log Message:
  -----------
  RecursiveASTVisitor: don't call WalkUp unnecessarily in post-order traversal

Summary:
How does RecursiveASTVisitor call the WalkUp callback for expressions?

* In pre-order traversal mode, RecursiveASTVisitor calls the WalkUp
  callback from the default implementation of Traverse callbacks.

* In post-order traversal mode when we don't have a DataRecursionQueue,
  RecursiveASTVisitor also calls the WalkUp callback from the default
  implementation of Traverse callbacks.

* However, in post-order traversal mode when we have a DataRecursionQueue,
  RecursiveASTVisitor calls the WalkUp callback from PostVisitStmt.

As a result, when the user overrides the Traverse callback, in pre-order
traversal mode they never get the corresponding WalkUp callback. However
in the post-order traversal mode the WalkUp callback is invoked or not
depending on whether the data recursion optimization could be applied.

I had to adjust the implementation of TraverseCXXForRangeStmt in the
syntax tree builder to call the WalkUp method directly, as it was
relying on this behavior. There is an existing test for this
functionality and it prompted me to make this extra fix.

In addition, I had to fix the default implementation implementation of
RecursiveASTVisitor::TraverseSynOrSemInitListExpr to call WalkUpFrom in
the same manner as the implementation generated by the DEF_TRAVERSE_STMT
macro. Without this fix, the InitListExprIsPostOrderNoQueueVisitedTwice
test was failing because WalkUpFromInitListExpr was never called.

Reviewers: eduucaldas, ymandel

Reviewed By: eduucaldas, ymandel

Subscribers: gribozavr2, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D82486


  Commit: c19c6b1722e5f71200c09cdb096245b98f03dce0
      https://github.com/llvm/llvm-project/commit/c19c6b1722e5f71200c09cdb096245b98f03dce0
  Author: Dmitri Gribenko <gribozavr at gmail.com>
  Date:   2020-07-06 (Mon, 06 Jul 2020)

  Changed paths:
    M clang/include/clang/AST/RecursiveASTVisitor.h
    M clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp

  Log Message:
  -----------
  Make RecursiveASTVisitor call WalkUpFrom for unary and binary operators in post-order traversal mode

Reviewers: ymandel, eduucaldas, rsmith

Reviewed By: eduucaldas, rsmith

Subscribers: gribozavr2, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D82787


  Commit: 8e750b1f0a2b6b5174dc49adf20e6f863c28e3cd
      https://github.com/llvm/llvm-project/commit/8e750b1f0a2b6b5174dc49adf20e6f863c28e3cd
  Author: Dmitri Gribenko <gribozavr at gmail.com>
  Date:   2020-07-06 (Mon, 06 Jul 2020)

  Changed paths:
    M clang/include/clang/AST/RecursiveASTVisitor.h
    M clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp

  Log Message:
  -----------
  Make RecursiveASTVisitor call WalkUpFrom for operators when the data recursion queue is absent

Reviewers: eduucaldas, ymandel, rsmith

Reviewed By: eduucaldas

Subscribers: gribozavr2, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D82889


  Commit: 5689b38c6a4220cc5f6ba68a56486229b10071bf
      https://github.com/llvm/llvm-project/commit/5689b38c6a4220cc5f6ba68a56486229b10071bf
  Author: Dmitri Gribenko <gribozavr at gmail.com>
  Date:   2020-07-06 (Mon, 06 Jul 2020)

  Changed paths:
    M clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp
    M clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h
    M clang/docs/ReleaseNotes.rst
    M clang/include/clang/AST/RecursiveASTVisitor.h
    M clang/lib/ARCMigrate/TransProperties.cpp
    M clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp

  Log Message:
  -----------
  Removed a RecursiveASTVisitor feature to visit operator kinds with different methods

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.

Reviewers: rsmith, sammccall, ymandel, aaron.ballman

Reviewed By: rsmith, sammccall, ymandel, aaron.ballman

Subscribers: cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D82921


Compare: https://github.com/llvm/llvm-project/compare/babbeafa006f...5689b38c6a42