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

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith 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;
----------------
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).



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109632/new/

https://reviews.llvm.org/D109632