rmaz added inline comments.
================ Comment at: clang/include/clang/Sema/Sema.h:1423-1426 + ObjCMethodList InstanceMethods; + ObjCMethodList FactoryMethods; + llvm::DenseSet<ObjCMethodDecl *> AddedInstanceMethods; + llvm::DenseSet<ObjCMethodDecl *> AddedFactoryMethods; ---------------- dexonsmith wrote: > Do these two sets really need to be per-selector (per `GlobalMethods` object > in GlobalMethodPool)? I feel like they could be global to `Sema`, as long as > the same `ObjCMethodDecl` is never added to GlobalMethodPool for more than > one selector. (It seems plausible we have that property since presumably > ObjCMethodDecl::getSelector is always the key used for inserting into > GlobalMethodPool below... can you confirm?) > > Assuming you can pull the sets out to represent the GlobalMethodPool as a > whole, then I suggest creating a data structure for GlobalMethodPool that > encapsulates the DenseMap and the two sets (maybe: rename "GlobalMethods" to > "GlobalMethodLists", rename "GlobalMethodPool" to "GlobalMethods", and give > the new data structure the name "GlobalMethodPool"). Best to do it as > multiple commits: NFC commit(s) to refactor (renames, create new type update > call sites to use new APIs, etc.), and a final commit that changes the > functionality (adding the set behaviour) but that doesn't need to touch call > sites. > > On the other hand, if the sets really need to be per-selector (please explain > why if so...), please use SmallPtrSet here instead of DenseSet to avoid > allocation in the common case of 1 decl per selector. I'd suggest > encapsulating the list/set together somehow (maybe starting with an NFC > refactoring to add a "list/set" data structure at the top-level (maybe rename > ObjCMethodList => ObjCMethodListNode, and the new list has a private member > called "Head" and necessary APIs for insertion/etc), then in a second commit > adding the SmallPtrSet into the list data structure and use it to gate the > "insert" operation). > > I feel like they could be global to Sema, as long as the same ObjCMethodDecl > is never added to GlobalMethodPool for more than one selector. (It seems > plausible we have that property since presumably ObjCMethodDecl::getSelector > is always the key used for inserting into GlobalMethodPool below... can you > confirm?) Yes, that is correct. We could also use a single set instead of two as the instance and factory methods would have different decls anyway. > Assuming you can pull the sets out to represent the GlobalMethodPool as a > whole, then I suggest creating a data structure for GlobalMethodPool that > encapsulates the DenseMap and the two sets I'll go with this approach with the single set, starting with a data structure refactor diff. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109632/new/ https://reviews.llvm.org/D109632 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
