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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
