Page MenuHomePhabricator

[clang][NFC] refactor GlobalMethodPool to encapsulate its map
ClosedPublic

Authored by rmaz on Sep 16 2021, 9:44 AM.

Details

Summary

This refactor changes the GlobalMethodPool to a class that contains
the DenseMap of methods. This is to allow for the addition of a
separate DenseSet in a follow-up diff that will handle method
de-duplication when inserting methods into the global method pool.

Changes:

  • the GlobalMethods pair becomes GlobalMethodPool::Lists
  • the GlobalMethodPool becomes a class containing the DenseMap of methods
  • pass through methods are added to maintain most of the existing code without changing MethodPool -> MethodPool.Methods everywhere

Diff Detail

Event Timeline

rmaz requested review of this revision.Sep 16 2021, 9:44 AM
rmaz created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2021, 9:45 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
dexonsmith accepted this revision.Sep 16 2021, 10:28 AM

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

clang/include/clang/Sema/Sema.h
1439–1444

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.

1445–1448

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:

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

1453

Nit: could use new-style using iterator = , but up to you.

1458

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)
1461–1462

Nit: these should be const.

This revision is now accepted and ready to land.Sep 16 2021, 10:28 AM
rmaz updated this revision to Diff 372999.Sep 16 2021, 11:01 AM
  • keep std::pair<ObjCMethodList, ObjCMethodList>, but move to GlobalMethodPool::Lists
  • add const
  • val -> &&Val
  • move comment
rmaz marked 5 inline comments as done.Sep 16 2021, 11:03 AM
rmaz edited the summary of this revision. (Show Details)Sep 16 2021, 12:02 PM
This revision was automatically updated to reflect the committed changes.