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

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

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;



================
Comment at: clang/include/clang/Serialization/ASTReader.h:1110
+  template <typename DeclTy>
+  using DuplicateDecls = std::pair<DeclTy *, DeclTy *>;
+
----------------
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".


================
Comment at: clang/include/clang/Serialization/ASTReader.h:1112-1115
+  /// When resolving duplicate ivars from extensions we don't error out
+  /// immediately but check if can merge identical extensions. Not checking
+  /// extensions for equality immediately because ivar deserialization isn't
+  /// over yet at that point.
----------------
ChuanqiXu wrote:
> Similarly, I suggest to add words or change the name to clarify it is used in ObjC only.
Thanks for sharing your perspective, I'll make it more obvious it is ObjC-related.


================
Comment at: clang/include/clang/Serialization/ASTReader.h:1117
+  llvm::MapVector<DuplicateDecls<ObjCCategoryDecl>,
+                  llvm::SmallVector<DuplicateDecls<ObjCIvarDecl>, 4>>
+      PendingExtensionIvarRedeclarations;
----------------
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.


================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:1249-1251
+    if (auto *ParentExt = dyn_cast<ObjCCategoryDecl>(IVD->getDeclContext())) {
+      if (auto *PrevParentExt =
+              dyn_cast<ObjCCategoryDecl>(PrevIvar->getDeclContext())) {
----------------
ChuanqiXu wrote:
> nit:
> ```
> auto *ParentExt = dyn_cast<ObjCCategoryDecl>(IVD->getDeclContext());
> auto *PrevParentExt = dyn_cast<ObjCCategoryDecl>(PrevIvar->getDeclContext());
> if (ParentExt && PrevParentExt) {
> 
> }
> ```
> 
> We could reduce one identation in this way. Codes with less identation looks better.
Thanks, good point, will change it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121177