[PATCH] D43494: [Modules] Fix creating fake definition data for lambdas.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:2969
 
+    if (!DD && RD->isBeingDefined())
+      return nullptr;
----------------
vsapsai wrote:
> vsapsai wrote:
> > aprantl wrote:
> > > Perhaps add a comment explaining what's going on in this early exit?
> > While I was trying to explain this early exit, I realized one of my assumptions was incorrect. Now I'm not sure returning nullptr is the right fix, maybe assigning incomplete definition data like
> > 
> > ```lang=c++
> > D->DefinitionData = DD;
> > ReadCXXDefinitionData(*DD, D);
> > ```
> > is better.
> > 
> > I am trying to come up with a test case showing different behaviour for these 2 different fixes. But so far nothing yet. If anybody can tell which one is correct: null context for merging or not-yet-populated definition data, that would be helpful.
> Looks like both fix approaches should work as in `ASTDeclReader::ReadCXXDefinitionData` we read decl only for lambda. If I'm not mistaken, for lambdas merging doesn't really make sense, so no context for merging doesn't have repercussions.
Merging for lambdas does make sense in the context of inline functions (where we can import two definitions of the same inline function from two different modules, and the definition can contain a lambda).

I think setting `D->DefinitionData` early would be the preferable solution. We should also disable the "all the bits must match" checks in `MergeDefinitionData` if one of the `DefinitionData` objects has `IsBeingDefined` set (or, ideally, defer those checks until recursive deserialization is complete).


https://reviews.llvm.org/D43494