[PATCH] D121177: [modules] Merge equivalent extensions and diagnose ivar redeclarations for extensions loaded from different modules.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D121177#3371695 <https://reviews.llvm.org/D121177#3371695>, @vsapsai wrote:

> Thanks for the review, Chuanqi! I believe you were touching recently `isSameEntity`. Do you have any concerns about using `StructuralEquivalenceContext` instead of `isSameEntity`? I've decided to go with `StructuralEquivalenceContext` because at this point we are still dealing with deserialization and deduplication while `ASTContext::isSameEntity` kinda assumes that all deduplication is already done, at least based on the comment
>
>   // Objective-C classes and protocols with the same name always match.
>   if (isa<ObjCInterfaceDecl>(X) || isa<ObjCProtocolDecl>(X))
>     return true;

Yeah, I was touching `isSameEntity`. But I don't know about `StructuralEquivalenceContext`. I just skipped over the implementation of `StructuralEquivalenceContext`. I feel like it would be OK if we could assume the input is duplicated indeed. 
I see it is guaranteed by `lookupInstanceVariable(Identifier)` now. I don't know much about ObjC. But I feel a little bit strange. Is it possible that two distinct variable with the same identifier in ObjC? If it is impossible, I think it might be OK.



================
Comment at: clang/include/clang/Serialization/ASTReader.h:1110
+  template <typename DeclTy>
+  using DuplicateDecls = std::pair<DeclTy *, DeclTy *>;
+
----------------
vsapsai wrote:
> ChuanqiXu wrote:
> > How about change the name to:
> > ```
> >   template <typename DeclTy>
> >   using DuplicateObjDecls = std::pair<DeclTy *, DeclTy *>;
> > ```
> > 
> > to clarify it is only used for ObjC. So the developer of other languages would skip this when reading.
> Would `DuplicateObjCDecls` work? `ObjC` part is common enough and I want to avoid just `Obj` because it can imply "Object" which is not the goal. Also we have `getObjCObjectType` which doesn't help with disambiguating the meaning of "Obj".
Yeah, it should be `ObjC`. It's my typo originally.


================
Comment at: clang/include/clang/Serialization/ASTReader.h:1117
+  llvm::MapVector<DuplicateDecls<ObjCCategoryDecl>,
+                  llvm::SmallVector<DuplicateDecls<ObjCIvarDecl>, 4>>
+      PendingExtensionIvarRedeclarations;
----------------
vsapsai wrote:
> ChuanqiXu wrote:
> > It looks a little bit odd for me to use `Vector` for duplicate vars. Especially, the structure is `Vector<pair<>>`. I feel it is better with `llvm::SmallSet`. If the order is important here, I think `llvm::SmallSetVector` might be an option.
> I'm not sure the use of `llvm::SmallSet` is warranted here. We deserialize each ObjCIvarDecl from each module (and corresponding DeclContext) only once, so we don't have to ensure there are no repeated pairs of same ObjCIvarDecl. And for stable diagnostic we need to use some kind of ordered container. Also a bunch of other "Pending..." fields don't use sets as uniqueness is enforced in different ways.
Oh, I got it. `llvm::SmallVector` might not be wrong in this case. And for the mapping relationship, `llvm::SmallMapVector` is considerable too.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121177/new/

https://reviews.llvm.org/D121177