Page MenuHomePhabricator

Extract ExprTraversal class from Expr
AbandonedPublic

Authored by steveire on Jan 20 2020, 4:06 AM.

Details

Reviewers
aaron.ballman
Summary

The current implementations are concerned with descending through the
AST. An addition to the API will be concerned with ascending through
the AST in different traversal modes. Keep both in the same class to
ensure that they are kept in sync.

Event Timeline

steveire created this revision.Jan 20 2020, 4:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2020, 4:06 AM

This looks like it's an NFC patch where the only change is to hoist this functionality out into its own interface. Is that correct? If so, can you please be sure to add NFC to the commit log?

clang/include/clang/AST/ExprTraversal.h
9

Some comments explaining the purpose of the file would be appreciated.

21

Any particular reason why this is a struct rather than a namespace given that there's no state?

22

Should we be adding some documentation comments for these APIs?

rsmith added a subscriber: rsmith.Jan 24 2020, 11:50 AM

An addition to the API will be concerned with ascending through the AST in different traversal modes.

Ascending through the AST is not possibly by design. (For example, we share AST nodes between template instantiations, and statement nodes don't contain parent pointers.) Can you say more about what you're thinking of adding here?

clang/include/clang/AST/ExprTraversal.h
1

This needs an update.

clang/lib/AST/ExprTraversal.cpp
1

This header should briefly describe the purpose of this file. ("Simple expression traversal"?)

steveire updated this revision to Diff 240267.Jan 24 2020, 12:30 PM

Update for review comments

steveire updated this revision to Diff 240269.Jan 24 2020, 12:33 PM
steveire marked an inline comment as done.

Update

steveire marked 4 inline comments as done.Jan 24 2020, 12:34 PM

An addition to the API will be concerned with ascending through the AST in different traversal modes.

Ascending through the AST is not possibly by design. (For example, we share AST nodes between template instantiations, and statement nodes don't contain parent pointers.) Can you say more about what you're thinking of adding here?

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 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 for related work in this area.

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 for related work in this area.

Thank you for pointing this out, I was unaware of that direction.

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 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 should be changed to implement the up-traversal-skipping directly in ParentMapContext.cpp

Is that the tight conclusion?

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 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 should be changed to implement the up-traversal-skipping directly in ParentMapContext.cpp

    Is that the right conclusion?

Implemented that in https://reviews.llvm.org/D73388

I think this and https://reviews.llvm.org/D73029 can be closed in favor of that one.

steveire abandoned this revision.Jan 26 2020, 11:15 AM