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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
