[PATCH] D64762: [AST] Treat semantic form of InitListExpr as implicit code in traversals

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:2332
       S->isSemanticForm() ? S->getSyntacticForm() : S, Queue));
   TRY_TO(TraverseSynOrSemInitListExpr(
       S->isSemanticForm() ? S : S->getSemanticForm(), Queue));
----------------
gribozavr wrote:
> ilya-biryukov wrote:
> > ilya-biryukov wrote:
> > > gribozavr wrote:
> > > > Instead of adding a whole new if statement, could you wrap the second existing TRY_TO in `if(shouldVisitImplicitCode())` ?
> > > Despite looking very similar, that would **not** be equivalent to the current version.
> > > 
> > > For most init lists (that do not have alternative "form"), the following invariants hold:
> > > ```
> > > InitList* E = ...;
> > > assert(E->isSemanticForm());
> > > assert(E->isSyntacticForm()); 
> > > assert(E->getSynacticForm() == nullptr);
> > > ```
> > > 
> > > This subtle fact means the current code does not traversed the list twice if they do not have an alternative form (either semantic or syntactic).
> > > 
> > > Now, if we only run the first statement, we will call `TraverseSynOrSemInitListExpr(S->getSyntacticForm())` and `S->getSyntacticForm()` returns `null`. So we don't traverse anything.
> > > 
> > > I tried various ways to keep both calls, but they all ended up being too complicated, hence the final version. Let me know if you see a better way to address this.
> > To make the last sentence less confusing:
> > I tried various ways to keep **only two** calls, but they were too complicated and I ended up introducing an extra call to `TraverseSyn...` instead.
> > 
> > assert(E->getSynacticForm() == nullptr);
> 
> That's... a really nice API.
> 
> What do you think about the following:
> 
> ```
> if (S->isSyntacticForm() && S->isSemanticForm()) {
>   // `S` does not have alternative forms, traverse the only form that's available.
>   TRY_TO(TraverseSynOrSemInitListExpr(S, Queue));
>   return true;
> }
> 
> TRY_TO(TraverseSynOrSemInitListExpr(
>   S->isSemanticForm() ? S->getSyntacticForm() : S, Queue));
> if (getDerived().shouldVisitImplicitCode()) {
>   TRY_TO(TraverseSynOrSemInitListExpr(
>     S->isSyntacticForm() ? S->getSemanticForm() : S, Queue));
> }
> ```
Definitely reads better, updated the revision accordingly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64762