vsapsai added a comment.
In D106994#2911508 <https://reviews.llvm.org/D106994#2911508>, @rsmith wrote:
> For what it's worth, I think the right way to handle this in C is to properly
> implement the "compatible types" rule instead of trying to invent something
> ODR-ish: in C, struct definitions in different translation units are
> different types, but if they're structurally equivalent then they're
> compatible and you can implicitly pass an object of some type to a function
> accepting a compatible type. This would mean that we could have multiple,
> different types with the same name, and we'd need name lookup to deduplicate
> compatible types, but we wouldn't need to do any cross-module ODR-like struct
> merging.
I agree that implementing the "compatible types" looks better as it models the
language more faithfully. And in the long run we might need to do that anyway.
Is there any work done for "compatible types" already? Or I can start by
creating a new type for a new definition with the same name and see how it
breaks the lookup?
From pragmatic perspective we are pretty invested into this ODR-ish approach
and it's not clear how much work switching to "compatible types" would take. So
I'd like to continue with the definition merging and evaluate the effort for
"compatible types". That's why I'm curious what work is done already.
> But assuming we want to keep the current ODR-in-C approach, this looks OK.
> There might be some places that assume the lexical and semantic `DeclContext`
> for a C `FieldDecl` are the same (etc) but I don't think there's a good way
> to find such things other than by testing this patch broadly.
Are there any known signs for mixing lexical and semantic `DeclContext`? I plan
to test the change on our internal codebase, hopefully it'll help to catch any
remaining issues.
================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:832
+ RD->setCompleteDefinition(false);
+ Reader.mergeDefinitionVisibility(OldDef, RD);
+ } else {
----------------
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);
```
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
