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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits