[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

Thu Oct 1 21:18:44 PDT 2020

lanza requested review of this revision.
lanza marked an inline comment as done.
lanza added inline comments.

================
Comment at: clang/lib/CodeGen/CGObjC.cpp:490
+  llvm::UniqueVector<ObjCProtocolDecl *> FoundProtocols;
+  std::set<ObjCProtocolDecl *> DeclaredProtocols;
+
----------------
rjmccall wrote:
> You should use llvm::DenseSet for this.  But in general there are more sets here than I think you really need, and you're building them eagerly when you don't have to.  I would suggest:
> 
> 1. Walk the list of declared protocols, adding the runtime protocols to the main result list and the first runtime protocols implied by the non-runtime protocols to a different list.
> 
> 2. If there are any protocols in the first-implied list (which is unlikely), build the implied-protocols set for all the protocols in the main list (inclusive of the protocols there) and the first-implied list (exclusive).  It would be totally reasonable to add a method to make this easier: `void ObjCProtocolDecl::getImpliedProtocols(llvm::DenseSet<const ObjCProtocolDecl*> &) const;`
> 
> 3. Add any protocols that are in the first-implied list but not the implied set back to the main list.
> 
> Also, protocols can be redeclared, so you should make sure to map to the canonical decl before adding them to a set (or UniqueVector).
Got ya, that should be much better for the 99.99% case where there are no non-runtime protocols. 👍

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75574