[PATCH] D73028: Extract ExprTraversal class from Expr

Stephen Kelly via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Jan 24 13:53:11 PST 2020


steveire added a comment.

In D73028#1839572 <https://reviews.llvm.org/D73028#1839572>, @rsmith wrote:

> In D73028#1839494 <https://reviews.llvm.org/D73028#1839494>, @steveire wrote:
>
> > In D73028#1839383 <https://reviews.llvm.org/D73028#1839383>, @rsmith wrote:
> >
> > >
> >
> >
> > The follow-up is here: https://reviews.llvm.org/D73029 .
> >
> > I have the need to change the ascending traversal implemented in ASTContext.cpp with a map (populated while descending) to make it skip nodes too during parent traversal.
> >
> > I didn't want to have descending traversal in Expr.cpp and ascending traversal in ASTContext.cpp as they would be likely to go out of sync, so moving this implementation here allows extending the purpose in the follow-up.
>
>
> The ascending traversal should not be in the AST library at all. If tooling or static analysis wants this, that's fine, but navigating upwards is not something the AST supports, and we do not want any part of clang proper developing a reliance on having a parent map, so we're working on moving the parent map out of `ASTContext` (with a plan to eventually move it out of the AST library). See D71313 <https://reviews.llvm.org/D71313> for related work in this area.


Thanks for the heads-up - I'm trying to reach the conclusion from this information.

Are you saying that

1. this patch shouldn't be committed, so down-traversal-skipping should remain in Expr.cpp
2. D73029 <https://reviews.llvm.org/D73029> should be changed to implement the up-traversal-skipping directly in ParentMapContext.cpp

Is that the tight conclusion?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73028/new/

https://reviews.llvm.org/D73028





More information about the cfe-commits mailing list