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