[PATCH] D109632: [clang] de-duplicate methods from AST files

[Richard Howell via Phabricator via cfe-commits]

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