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

[rmaz]

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

[dexonsmith]

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.

[rmaz]

• keep std::pair<ObjCMethodList, ObjCMethodList>, but move to GlobalMethodPool::Lists
• add const
• val -> &&Val
• move comment

clang/include/clang/Sema/Sema.h

   void getUndefinedButUsed(
       SmallVectorImpl<std::pair<NamedDecl *, SourceLocation> > &Undefined);
 
   /// Retrieves list of suspicious delete-expressions that will be checked at
   /// the end of translation unit.
   const llvm::MapVector<FieldDecl *, DeleteLocs> &
   getMismatchingDeleteExpressions() const;
 
-  typedef std::pair<ObjCMethodList, ObjCMethodList> GlobalMethods;
-  typedef llvm::DenseMap<Selector, GlobalMethods> GlobalMethodPool;
+  class GlobalMethodPool {
+  public:
+    using Lists = std::pair<ObjCMethodList, ObjCMethodList>;
+    using iterator = llvm::DenseMap<Selector, Lists>::iterator;
+    iterator begin() { return Methods.begin(); }
+    iterator end() { return Methods.end(); }
+    iterator find(Selector Sel) { return Methods.find(Sel); }
+    std::pair<iterator, bool> insert(std::pair<Selector, Lists> &&Val) {
+      return Methods.insert(Val);
+    }
+    int count(Selector Sel) const { return Methods.count(Sel); }
+    bool empty() const { return Methods.empty(); }
+
+  private:
+    llvm::DenseMap<Selector, Lists> Methods;
+  };
 
   /// 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.
   GlobalMethodPool MethodPool;
 
   /// Method selectors used in a \@selector expression. Used for implementation
   /// of -Wselector.
   llvm::MapVector<Selector, SourceLocation> ReferencedSelectors;
 
   /// List of SourceLocations where 'self' is implicitly retained inside a
   /// block.
   llvm::SmallVector<std::pair<SourceLocation, const BlockDecl *>, 1>
       ImplicitlyRetainedSelfLocs;
 
   /// Kinds of C++ special members.
   enum CXXSpecialMember {
     CXXDefaultConstructor,
     CXXCopyConstructor,
     CXXMoveConstructor,
     CXXCopyAssignment,
     CXXMoveAssignment,
     CXXDestructor,
     CXXInvalid
   };
 
   typedef llvm::PointerIntPair<CXXRecordDecl *, 3, CXXSpecialMember>
       SpecialMemberDecl;
 
   /// The C++ special members which we are currently in the process of

clang/lib/Sema/SemaDeclObjC.cpp

void Sema::AddMethodToGlobalPool(ObjCMethodDecl *Method, bool impl,
   if (cast<Decl>(Method->getDeclContext())->isInvalidDecl())
     return;
 
   if (ExternalSource)
     ReadMethodPool(Method->getSelector());
 
   GlobalMethodPool::iterator Pos = MethodPool.find(Method->getSelector());
   if (Pos == MethodPool.end())
-    Pos = MethodPool.insert(std::make_pair(Method->getSelector(),
-                                           GlobalMethods())).first;
+    Pos = MethodPool
+              .insert(std::make_pair(Method->getSelector(),
+                                     GlobalMethodPool::Lists()))
+              .first;
 
   Method->setDefined(impl);
 
   ObjCMethodList &Entry = instance ? Pos->second.first : Pos->second.second;
   addMethodToGlobalList(&Entry, Method);
 }
 
 /// Determines if this is an "acceptable" loose mismatch in the global
void Sema::DiagnoseMultipleMethodInGlobalPool(SmallVectorImpl<ObjCMethodDecl*> &Methods,
   }
 }
 
 ObjCMethodDecl *Sema::LookupImplementedMethodInGlobalPool(Selector Sel) {
   GlobalMethodPool::iterator Pos = MethodPool.find(Sel);
   if (Pos == MethodPool.end())
     return nullptr;
 
-  GlobalMethods &Methods = Pos->second;
+  GlobalMethodPool::Lists &Methods = Pos->second;
   for (const ObjCMethodList *Method = &Methods.first; Method;
        Method = Method->getNext())
     if (Method->getMethod() &&
         (Method->getMethod()->isDefined() ||
          Method->getMethod()->isPropertyAccessor()))
       return Method->getMethod();
 
   for (const ObjCMethodList *Method = &Methods.second; Method;

clang/lib/Serialization/ASTReader.cpp

if (Visitor.getInstanceMethods().empty() &&
     return;
 
   ++NumMethodPoolHits;
 
   if (!getSema())
     return;
 
   Sema &S = *getSema();
-  Sema::GlobalMethodPool::iterator Pos
-    = S.MethodPool.insert(std::make_pair(Sel, Sema::GlobalMethods())).first;
+  Sema::GlobalMethodPool::iterator Pos =
+      S.MethodPool.insert(std::make_pair(Sel, Sema::GlobalMethodPool::Lists()))
+          .first;
 
   Pos->second.first.setBits(Visitor.getInstanceBits());
   Pos->second.first.setHasMoreThanOneDecl(Visitor.instanceHasMoreThanOneDecl());
   Pos->second.second.setBits(Visitor.getFactoryBits());
   Pos->second.second.setHasMoreThanOneDecl(Visitor.factoryHasMoreThanOneDecl());
 
   // Add methods to the global pool *after* setting hasMoreThanOneDecl, since
   // when building a module we keep every method individually and may need to