rsmith added inline comments.
================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:832
+ RD->setCompleteDefinition(false);
+ Reader.mergeDefinitionVisibility(OldDef, RD);
+ } else {
----------------
vsapsai wrote:
> rsmith wrote:
> > vsapsai wrote:
> > > Here is the perfect place to compare if `RD` and `OldDef` are equivalent
> > > and emit diagnostic if they are not. I've tried
> > > `StructuralEquivalenceContext` for this purpose but it compares canonical
> > > decls which doesn't work in this case. I think the best approach for this
> > > task would be ODR hash comparison proposed in
> > > https://reviews.llvm.org/D71734 It will need some tweaks to work with the
> > > current patch but overall the plan is to use ODR hash instead of any
> > > other decl comparison.
> > Just a minor note: it's not safe to emit diagnostics from here in general;
> > in particular, emitting a diagnostic that refers to a declaration can
> > trigger deserialization, which can reenter the AST reader in unfortunate
> > ways and crash. But we can make a note to do the structural equivalence
> > check here and then actually perform the check when we finish
> > deserialization (with the other merging checks).
> Thanks for pointing it out, I didn't realize diagnostic can trigger
> deserialization. Was planning to do something like
>
> ```lang=c++
> if (OldDef->getODRHash() != RD->getODRHash())
> Reader.PendingRecordOdrMergeFailures[OldDef].push_back(RD);
> ```
That seems reasonable to me.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D106994/new/
https://reviews.llvm.org/D106994
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
