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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
  • [PATCH] D10989... Richard Howell via Phabricator via cfe-commits
    • [PATCH] D... Duncan P. N. Exon Smith via Phabricator via cfe-commits
    • [PATCH] D... Richard Howell via Phabricator via cfe-commits
    • [PATCH] D... Daniel Rodríguez Troitiño via Phabricator via cfe-commits

Reply via email to