[PATCH] D109898: [clang][NFC] refactor GlobalMethodPool to encapsulate its map

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

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

Thanks for splitting this out -- left some comments inline, LGTM after that.



================
Comment at: clang/include/clang/Sema/Sema.h:1422-1427
   /// Method Pool - allows efficient lookup when typechecking messages to "id".
   /// We need to maintain a list, since selectors can have differing signatures
   /// across classes. In Cocoa, this happens to be extremely uncommon (only 1%
   /// of selectors are "overloaded").
   /// At the head of the list it is recorded whether there were 0, 1, or >= 2
   /// methods inside categories with a particular selector.
----------------
This comment should probably be attached to either GlobalMethodPool (the type) or MethodPool (the field), not this helper struct. I'd probably leave it attached to the field since that's where it was before.


================
Comment at: clang/include/clang/Sema/Sema.h:1428-1431
+  struct GlobalMethodLists {
+    ObjCMethodList InstanceMethods;
+    ObjCMethodList ClassMethods;
+  };
----------------
Switching away from std::pair adds a lot of churn that's not related to encapsulating the pool -- it seems like it touches mostly new lines of code, not much crossover. Unless I misdiagnosed, then I think it'd be better to skip this change (for now), leaving as a typedef:
```
lang=c++
using GlobalMethodLists = std::pair<ObjCMethodList, ObjCMethodList>;
```
(Or, I wonder, should the typedef be moved inside of `GlobalMethodPool` (maybe `GlobalMethodPool::Lists`?) to tidy things up further? Up to you.)

(To be clear, switching to a `struct` seems like a great change, I just think it'd be better to commit separately. (In that commit, I suggest shorter names for the fields -- "Instances" and "Classes" -- but up to you.))


================
Comment at: clang/include/clang/Sema/Sema.h:1436
+  public:
+    typedef llvm::DenseMap<Selector, GlobalMethodLists>::iterator iterator;
+    iterator begin() { return Methods.begin(); }
----------------
Nit: could use new-style `using iterator = `, but up to you.


================
Comment at: clang/include/clang/Sema/Sema.h:1441
+    std::pair<iterator, bool>
+    insert(std::pair<Selector, GlobalMethodLists> val) {
+      return Methods.insert(val);
----------------
A couple of nits:
- `val` should probably be `Val` or `V`
- I think this should be `&&`; it's not obvious from here that ObjCMethodList has a cheap copy constructor (it does, but)


================
Comment at: clang/include/clang/Sema/Sema.h:1444-1445
+    }
+    int count(Selector Sel) { return Methods.count(Sel); }
+    bool empty() { return Methods.empty(); }
+  };
----------------
Nit: these should be `const`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109898