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