[PATCH] D52267: [AST] Various optimizations + refactoring in DeclarationName(Table)

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Conceptually, this change looks great.  And it should be fine to require extra alignment on `IdentifierInfo` on 32-bit hosts; I doubt that will have measurable impact.

I believe it's possible to eliminate the need for most, perhaps all, of these `static_asserts` by just defining the constants more sensibly in the first place.



================
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 {
+
----------------
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.


================
Comment at: include/clang/AST/DeclarationName.h:54
+/// we use three different FoldingSet<CXXSpecialName> in DeclarationNameTable.
+class alignas(8) CXXSpecialName : public llvm::FoldingSetNode {
+  friend class clang::DeclarationName;
----------------
If this isn't a self-contained description of the name anymore, I think adding `Extra` to the class name would be appropriate.  And maybe the class name should directly express that is for the kinds of names that are basically uniqued by type.

Please introduce a symbolic constant for 8 here, since you use and assume it across multiple places.  Either `#define` or a `const` variable works.


================
Comment at: include/clang/AST/DeclarationName.h:98
+/// Contains extra information for the name of an overloaded
+/// operator in C++, such as "operator+.
+class alignas(8) CXXOperatorIdName {
----------------
This is probably pre-existing, but the comment should clarify that this doesn't include literal or conversion operators.


================
Comment at: include/clang/AST/DeclarationName.h:164
+    CXXUsingDirective = 10,
+    ObjCMultiArgSelector = 11
+  };
----------------
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.


================
Comment at: include/clang/AST/DeclarationName.h:178
+  ///   This enable efficient conversion between the two enumerations
+  ///   in the usual case.
+  ///
----------------
You can express this directly by defining `StoredNameKind` first and then defining the corresponding enumerators in `NameKind` in terms of them.


================
Comment at: include/clang/AST/DeclarationName.h:184
+  ///   between DeclarationNameExtra::ExtraKind and NameKind possible with
+  ///   a single addition/substraction.
+  ///
----------------
Same deal.  Just define the enumerators of `NameKind` appropriately.


================
Comment at: include/clang/AST/DeclarationName.h:243
+  ///   C++ literal operator, or C++ using directive.
   uintptr_t Ptr = 0;
 
----------------
riccibruno wrote:
> erichkeane wrote:
> > There is an llvm type for storing something in the lower bits of a pointer.  I THINK it is llvm::PointerIntPair.
> > 
> > I'd MUCH prefer you do that instead of the reimplementation here.
> Well it was already like this but point taken.
Using `PointerIntPair` probably won't work here because the pointer type is basically a union, and you can't use `void*` because `PointerIntPair` wants to assert that the pointer type is sufficiently aligned.


Repository:
  rC Clang

https://reviews.llvm.org/D52267