[PATCH] D50947: lambdas in modules: make PendingFakeDefinitionData a set

[Steve O'Brien via Phabricator via cfe-commits]

elsteveogrande added inline comments.


================
Comment at: lib/Serialization/ASTReaderDecl.cpp:1766
+
+    // FIXME: handle serialized lambdas
     assert(!DD.IsLambda && !MergeDD.IsLambda && "faked up lambda definition?");
----------------
rsmith wrote:
> Did you mean to add this FIXME?
Yup: to indicate this is assert about `faked up lambda definition` will be addressed in an upcoming diff, i.e., fixing the lambda issues :)  See D50948, D50949


================
Comment at: lib/Serialization/ASTReaderDecl.cpp:4246
           OldDD && (OldDD->Definition != RD ||
                     !Reader.PendingFakeDefinitionData.count(OldDD));
       RD->setParamDestroyedInCallee(Record.readInt());
----------------
rsmith wrote:
> This does not appear to be NFC: the `FakeLoaded` vs absent-from-map distinction matters here. I think the idea here is that we still need to load the lexical contents of the declaration of the class that we pick to be the definition even if we've already found and merged another definition into it.
> 
> That said, if this //is// necessary, we should have some test coverage for it, and based on this change not breaking any of our tests, we evidently don't. :( I'll try this change out against our codebase and see if I can find a testcase.
Thanks for checking this!  Yup I missed this; count would be different and therefore this changes logic, which surprisingly didn't break things as far as I could see.  I'll change this back to `Fake` vs. `FakeLoaded`.


Repository:
  rC Clang

https://reviews.llvm.org/D50947