rjmccall accepted this revision.
rjmccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: include/clang/AST/DeclarationName.h:46
 
-/// DeclarationName - The name of a declaration. In the common case,
-/// this just stores an IdentifierInfo pointer to a normal
-/// name. However, it also provides encodings for Objective-C
-/// selectors (optimizing zero- and one-argument selectors, which make
-/// up 78% percent of all selectors in Cocoa.h) and special C++ names
-/// for constructors, destructors, and conversion functions.
+namespace detail {
+
----------------
riccibruno wrote:
> rjmccall wrote:
> > Should `DeclarationNameExtra` be moved here?  I'm not sure why it's in 
> > `IdentifierTable.h` in the first place, and your refactor seems to make 
> > that even more pointless.
> `DeclarationNameExtra` is unfortunately needed by `MultiKeywordSelector`
> in `IdentifierInfo.cpp`.
Oh, and one is in Basic instead of AST.

I'm not sure there's really a good reason for Selector to exist at the Basic 
level instead of AST; it's probably more an artifact of old divisions than 
something that's really useful.  But changing that would probably be painful.


================
Comment at: include/clang/AST/DeclarationName.h:164
+    CXXUsingDirective = 10,
+    ObjCMultiArgSelector = 11
+  };
----------------
riccibruno wrote:
> rjmccall wrote:
> > Is the criterion for inclusion in the first seven really just frequency of 
> > use, or is it a combination of that and relative benefit?
> > 
> > The only one I would really quibble about is that multi-argument selectors 
> > are probably more common and important to Objective-C code than conversion 
> > operators are to C++.  But it's quite possible that the relative benefit is 
> > much higher for C++, since selectors only appear on specific kinds of 
> > declarations that know they're declared with selectors — relatively little 
> > code actually needs to be polymorphic about them — and since they have to 
> > be defined out-of-line.
> I did not do an in-depth analysis regarding the selection of the first seven 
> inline kinds.
> My thought process was that C++ constructors and operator names should 
> definitely be
> inline. C++ destructors seemed much more frequent than c++ literal operators 
> and
> deduction guides. This leaves one slot available and since C++ conversion 
> operators
> share the same class `CXXSpecialName` including it as an inline kind 
> simplifies the code.
> 
> Also a practical point is that I don't really have a representative sample of 
> ObjC code
> to benchmark this. For C++ I am using all of Boost which I hope is 
> representative
> enough. If you deem it necessary I will try to do some benchmarks with
> `ObjCMultiArgSelector` stored inline.
I think I can accept this as-is without extra investigation.


================
Comment at: include/clang/AST/DeclarationName.h:220
+                           detail::DeclarationNameExtra::ObjCMultiArgSelector
   };
 
----------------
Thanks, this looks great.


Repository:
  rC Clang

https://reviews.llvm.org/D52267



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to