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