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