Meinersbur added inline comments.
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:5215
+ Expr *NegIncAmount =
+ AssertSuccess(Actions.BuildUnaryOp(nullptr, {}, UO_Minus, NewStep));
+ Expr *BackwardDist = AssertSuccess(
----------------
Meinersbur wrote:
> jdenny wrote:
> > It looks like the AST nodes `NewStart`, `NewStop`, and `NewStep` are shared
> > by multiple parent nodes here. Does that ever happen anywhere else in a
> > Clang AST?
> Yes. For instance, when instantiating a template, nodes form the
> TemplatedDecl are reused unless they need to change. Therefore multiple
> template instantiation can also share entire AST subtrees. Why rebuild them
> if they are identical?
>
> However, I also found the following description of
> TreeTransform::AlwaysRebuild():
> ```
> /// We must always rebuild all AST nodes when performing variadic template
> /// pack expansion, in order to avoid violating the AST invariant that each
> /// statement node appears at most once in its containing declaration.
> ```
> It was added by @rsmith in rG2aa81a718e64fb4ca651ea12ab7afaeffc11e463 to fix
> llvm.org/PR17800 . The assertion doesn't seem to exist in the current code
> base.
>
> Does this apply here?
I think the invariant exists because codegen has a map from ast expression
nodes to its temporary memory location. This means that reusing the same AST
expression node indeed should not be done.
I think the assertion that failed in llvm.org/PR17800 is now here:
https://github.com/llvm/llvm-project/blob/7ba2e1c6011eeb1b91ce3f5d8fa7187b7518e77a/clang/lib/AST/ExprConstant.cpp#L1872
Assertion message was changed in this commit:
https://github.com/llvm/llvm-project/commit/f7f2e4261a98b2da519d58e7f6794b013cda7a4b
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D94973/new/
https://reviews.llvm.org/D94973
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
