[PATCH] D86207: (De-)serialize BindingDecls before DecompositionDecl

Wed Sep 2 08:57:22 PDT 2020

aaronpuchert added a comment.

In D86207#2251802 <https://reviews.llvm.org/D86207#2251802>, @riccibruno wrote:

> Is my comment on the deserialization of `BindingDecl`s in https://reviews.llvm.org/D85613?id=284364 related to this change?

Not sure. The `FIXME` comment on the code is correct, and it would be correct after this change. But notice that `Decomp` is also not set when constructing a `BindingDecl` from the parser: first we build the `BindingDecl`s in `Sema::ActOnDecompositionDeclarator` (there `Decomp` remains unset), then build the `DecompositionDecl` from that in `Sema::ActOnVariableDeclarator`. The constructor of `DecompositionDecl` is then calling `setDecomposedDecl` on all bindings, so `Decomp` is set as soon as that's finished. But the `BindingDecl`s exist without `Decomp` for a while.

In D85613#2210054 <https://reviews.llvm.org/D85613#2210054>, @riccibruno wrote:

> The expression for the binding (`Binding`) will be deserialized when visiting the `BindingDecl`. This expression when non-null will always (as far as I can tell) contain a reference to the decomposition declaration so the decomposition will be deserialized which will set `Decomp`.

My impression is that `Decomp` is set by `ASTDeclReader::VisitDecompositionDecl`, after reading the individual `BindingDecl`s, not from deserializing the `Binding` expression. Otherwise the `BDs[I]->setDecomposedDecl(DD);` above wouldn't be needed. But maybe I'm misunderstanding you.

Maybe you're saying it might be a problem if I read the `BindingDecls` first and then they can't reference the `DecompositionDecl`? I'm not sure how I can see the deserialized AST, when I use `-ast-dump` with `-include-pch` it doesn't show the AST of the included header. But the generated code looks fine for an example I tried out.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86207