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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
