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

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

elsteveogrande abandoned this revision.
elsteveogrande added inline comments.


================
Comment at: lib/Serialization/ASTReaderDecl.cpp:1760-1765
-  if (PFDI != Reader.PendingFakeDefinitionData.end() &&
-      PFDI->second == ASTReader::PendingFakeDefinitionKind::Fake) {
+  if (PFDI != Reader.PendingFakeDefinitionData.end()) {
     // We faked up this definition data because we found a class for which we'd
     // not yet loaded the definition. Replace it with the real thing now.
+
+    Reader.PendingFakeDefinitionData.erase(PFDI);
+
+    // FIXME: handle serialized lambdas
     assert(!DD.IsLambda && !MergeDD.IsLambda && "faked up lambda definition?");
-    PFDI->second = ASTReader::PendingFakeDefinitionKind::FakeLoaded;
----------------
(2) the `find`, and the flip to `FakeLoaded`


================
Comment at: lib/Serialization/ASTReaderDecl.cpp:3080-3081
       // Track that we did this horrible thing so that we can fix it later.
-      Reader.PendingFakeDefinitionData.insert(
-          std::make_pair(DD, ASTReader::PendingFakeDefinitionKind::Fake));
     }
----------------
(1) the insert


================
Comment at: lib/Serialization/ASTReaderDecl.cpp:4246
           OldDD && (OldDD->Definition != RD ||
                     !Reader.PendingFakeDefinitionData.count(OldDD));
       RD->setParamDestroyedInCallee(Record.readInt());
----------------
elsteveogrande wrote:
> 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`.
Hmm.  In the lambda case, the `FakeLoaded` remains in the table, and we get "faked up a class definition but never saw the real one":

  assert(PendingFakeDefinitionData.empty() &&
         "faked up a class definition but never saw the real one");

Failing assert here: [lib/Serialization/ASTReader.cpp](https://github.com/llvm-mirror/clang/blob/master/lib/Serialization/ASTReader.cpp#L9347)

So I'm digging into why `PendingFakeDefinitionData` isn't emptying out.  Recapping the logic in this class (to make sure I know what's going on here / to help explain it to myself)

1. a `Fake` insert happens around 3081, after creating a fake/empty `DefinitionData` and setting on a new `CXXRecordDecl` (and its canonical decl if different)
2. this table is checked in `MergeDefinitionData` from 1765; if there's a `Fake` entry, flip it to `FakeLoaded`
3. the only removal that I could find is when a decl is updated with `UPD_CXX_INSTANTIATED_CLASS_DEFINITION` around line 4256, only if there wasn't an update and there's a lexical DC.  (In any event the `UPD_CXX_INSTANTIATED_...` wouldn't fire in my case)



================
Comment at: lib/Serialization/ASTReaderDecl.cpp:4255
         Record.readLexicalDeclContextStorage(LexicalOffset, RD);
         Reader.PendingFakeDefinitionData.erase(OldDD);
       }
----------------
(3) ... and finally the erase


Repository:
  rC Clang

https://reviews.llvm.org/D50947