rsmith accepted this revision.
rsmith added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:1181-1184
+ if (DD.Definition != NewDD.Definition) {
+ Reader.MergedDeclContexts.insert(
+ std::make_pair(NewDD.Definition, DD.Definition));
+ }
----------------
vsapsai wrote:
> 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.
OK, separating out those changes with separate tests makes sense to me. Thanks!
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
