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

Bruno Ricci via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Sep 2 09:46:29 PDT 2020


riccibruno added a comment.

In D86207#2252409 <https://reviews.llvm.org/D86207#2252409>, @aaronpuchert wrote:

> 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.

I agree, but I think that the `BindingDecl` should still be considered to be under construction until the corresponding `DecompositionDecl` is constructed. Any use of the `BindingDecl` in this interval has to be mindful that the `BindingDecl` is not fully constructed.

> 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.

My concern is:

Is it possible to see a deserialized `BindingDecl` when  the corresponding `DecompositionDecl` has not been deserialized (which will as you say set `BindingDecl::Decomp`)? I have not been able to construct an example where this is the case but I am not at all confident that this is impossible.

(`-ast-dump-all` can be added to force deserialization)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86207



More information about the cfe-commits mailing list