vsapsai added inline comments.
================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:1181-1184
+ if (DD.Definition != NewDD.Definition) {
+ Reader.MergedDeclContexts.insert(
+ std::make_pair(NewDD.Definition, DD.Definition));
+ }
----------------
rsmith wrote:
> A couple of other things you should consider doing in this case:
>
> * If there's an AST invariant that there is only one definition, think about
> how to maintain that invariant. For other kinds of declaration, we would
> update `NewDD` to make it act as a forward-declaration, but I'm not sure we
> can do much about it here given that an `@interface` is never a
> forward-declaration. Maybe for `@interface` there's nothing we can really do,
> and code dealing with these declarations just needs to handle there possibly
> being more than one definition?
> * Merge the definition visibility of the new definition into the canonical
> definition. This is necessary to ensure that in code for which the new
> definition is imported and the canonical definition is not, we still treat
> the canonical definition as being visible. (Eg, module A has the canonical
> definition, module B has another definition, module C imports A but doesn't
> re-export it, and C also imports B and re-exports it, and then you import C
> and try to use the class.)
>
> For the latter, `Reader.mergeDefinitionVisibility(DD.Definition,
> NewDD.Definition)` should be sufficient, assuming that all code treats the
> first definition as being the canonical one (for example, name lookups are
> performed into the first definition).
Change to merge visibility is D110453 and you are right, the code is simple,
the main complexity is the test.
As for maintaining a single definition, we are already doing that (see my
comment a few lines below in `ASTDeclReader::VisitObjCInterfaceDecl`). But not
doing good job with `ObjCInterfaceType::getDecl`. The fix for that is D110452
but the summary is that `ObjCInterfaceType` stores the last encountered
definition, it is never updated when we discard this definition, that
not-so-definition-anymore is treated as a canonical definition.
Updating visibility doesn't work without D110452 because we might end up
looking up a different "definition", not the one with correct[ed] visibility.
And D110452 seems to be different enough, so I've split it out as a separate
patch. I've made current change a separate patch for clear indication what code
changes lead to changes in clang/test/Modules/odr_hash.mm, there is no
technical reason it cannot be merged with other changes.
Also I should note that `@interface` tracking story isn't over yet, I still
need to check and write tests for merging categories. They cannot have ivars,
so don't expect IRGen issues but there is still enough room for other bugs.
================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:1202-1206
if (Canon->Data.getPointer()) {
// If we already have a definition, keep the definition invariant and
// merge the data.
MergeDefinitionData(Canon, std::move(ID->data()));
ID->Data = Canon->Data;
----------------
Here we keep a single definition. Have a last chance to keep extra information
from the second definition but afterwards just overwrite `ID->Data`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D110287/new/
https://reviews.llvm.org/D110287
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
