[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.

I don't know about ObjC.  Comments on style/name only.



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


================
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.
----------------
Similarly, I suggest to add words or change the name to clarify it is used in ObjC only.


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


================
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())) {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121177