[PATCH] D11298: Convert a few classes over to use the new TrailingObjects helper.

[James Y Knight]

jyknight marked an inline comment as done.

================
Comment at: include/clang/AST/Decl.h:3620
@@ +3619,3 @@
+      public DeclContext,
+      llvm::TrailingObjects<CapturedDecl, ImplicitParamDecl *> {
+protected:
----------------
rsmith wrote:
> I would prefer to see an explicit `private` access specifier here.
Done here and everywhere.

================
Comment at: include/clang/AST/Decl.h:3702
@@ -3691,2 +3701,3 @@
   friend class ASTDeclWriter;
+  friend class TrailingObjects;
 };
----------------
rsmith wrote:
> I predict one of our supported host compilers will choke on this.
> 
> *experiments*
> 
> That compiler is MSVC. It appears all our supported compilers provide enough C++11 to allow
> 
>   friend TrailingObjects;
Done here and everywhere

================
Comment at: include/clang/AST/DeclTemplate.h:47
@@ -45,2 +46,3 @@
 /// derived classes.
-class LLVM_ALIGNAS(/*alignof(void*)*/ LLVM_PTR_SIZE) TemplateParameterList {
+class LLVM_ALIGNAS(/*alignof(void*)*/ LLVM_PTR_SIZE) TemplateParameterList final
+    : llvm::TrailingObjects<TemplateParameterList, NamedDecl *> {
----------------
rsmith wrote:
> Can you remove the LLVM_ALIGNAS?
Nope, still needs to be provded manually. The TrailingObjects class does not currently adjust the alignment for you, but rather, just static_asserts that you've gotten it right. I'd like it to, but right now, the argument to LLVM_ALIGNAS apparently needs to be a literal integer for MSVC.

...although...actually, now that I think about it more, it may actually be possible to make it work even on VC. I don't have a way to test it, but I think something like the following might do the trick.

I'd like to avoid trying to add this feature to the llvm TrailingObjects class right now, though, and try it in a follow-up later on, OK?

```
template<int Align> class AlignmentHelper {};

template<> class LLVM_ALIGNAS(1) AlignmentHelper<1> {};
template<> class LLVM_ALIGNAS(2) AlignmentHelper<2> {};
template<> class LLVM_ALIGNAS(4) AlignmentHelper<4> {};
template<> class LLVM_ALIGNAS(8) AlignmentHelper<8> {};
template<> class LLVM_ALIGNAS(16) AlignmentHelper<16> {};
template<> class LLVM_ALIGNAS(32) AlignmentHelper<32> {};

class OtherThing {
  double x;
};

class SomeThing : AlignmentHelper<AlignOf<OtherThing>::Alignment> {
  char x;
};

int main() {
  static_assert(AlignOf<OtherThing>::Alignment == AlignOf<SomeThing>::Alignment, "");
}

```

================
Comment at: include/clang/AST/DeclTemplate.h:145
@@ -136,4 +144,3 @@
 /// derived classes. Suitable for creating on the stack.
-template<size_t N>
-class FixedSizeTemplateParameterList : public TemplateParameterList {
-  NamedDecl *Params[N];
+template <size_t N> class FixedSizeTemplateParameterListStorage {
+  char Mem[TemplateParameterList::totalSizeToAlloc<NamedDecl *>(N)];
----------------
rsmith wrote:
> This type is now underaligned.
Indeed! stupid oversight from adjusting the code at the last minute for the required "final". Should've inserted an alignment here too. But because of the constexpr issue, this goes away anyways...

================
Comment at: include/clang/AST/DeclTemplate.h:146
@@ -140,1 +145,3 @@
+template <size_t N> class FixedSizeTemplateParameterListStorage {
+  char Mem[TemplateParameterList::totalSizeToAlloc<NamedDecl *>(N)];
 
----------------
rnk wrote:
> rsmith wrote:
> > Do all of our supported compilers provide enough `constexpr` support for this to actually work? How would you feel about:
> > 
> >   TemplateParameterList List;
> >   NamedDecl *Params[N];
> > 
> > (Yeah, I don't like this either.)
> Won't this reduce the alignment of this structure to 1?
Yeah, right, no constexpr in VS2013. So, I guess that'll have to do...yuck but oh well.

I stuck in some asserts for the layout being as expected to assuage my conscience.

================
Comment at: include/clang/AST/DeclTemplate.h:165
@@ -151,6 +164,3 @@
   /// \brief The template argument list.
-  ///
-  /// The integer value will be non-zero to indicate that this
-  /// template argument list does own the pointer.
-  llvm::PointerIntPair<const TemplateArgument *, 1> Arguments;
+  const TemplateArgument *Arguments;
 
----------------
rsmith wrote:
> Please check in the removal of the `Owned` flag separately.
Acknowledged. [but still in this change as of right now]

================
Comment at: include/clang/AST/DeclTemplate.h:1290-1302
@@ -1262,15 +1289,15 @@
   /// pack.
   QualType getExpansionType(unsigned I) const {
     assert(I < NumExpandedTypes && "Out-of-range expansion type index");
-    void * const *TypesAndInfos = reinterpret_cast<void * const*>(this + 1);
+    void *const *TypesAndInfos = getTrailingObjects<void *>();
     return QualType::getFromOpaquePtr(TypesAndInfos[2*I]);
   }
 
   /// \brief Retrieve a particular expansion type source info within an
   /// expanded parameter pack.
   TypeSourceInfo *getExpansionTypeSourceInfo(unsigned I) const {
     assert(I < NumExpandedTypes && "Out-of-range expansion type index");
-    void * const *TypesAndInfos = reinterpret_cast<void * const*>(this + 1);
+    void *const *TypesAndInfos = getTrailingObjects<void *>();
     return static_cast<TypeSourceInfo *>(TypesAndInfos[2*I+1]);
   }
 
----------------
rsmith wrote:
> Wow, this is still pretty gross. Can we use `std::pair<QualType, TypeSourceInfo*>` as the trailing object type, or something morally equivalent?
Done (could also use two trailing arrays, of QualType and separately of TypeSourceInfo*, but this is closer to the current code)

================
Comment at: lib/AST/Decl.cpp:3129
@@ +3128,3 @@
+  void *Buffer = Context.Allocate(
+      totalSizeToAlloc<TemplateArgumentLoc, FunctionTemplateDecl *>(
+          TArgs.size(), Ts.size()));
----------------
rsmith wrote:
> Why do you need to specify the types here? Shouldn't they be implied by the type of the `TrailingObjects` base class?
I made the API require them, because I didn't like the interface with the unadorned integers in an arbitrary order. It just seemed too easy to forget to update these, if you swapped the order of the types in the class, e.g. for alignment reasons.

So, {additional,total}SizeToAlloc requires the types be presented here, redundantly.


http://reviews.llvm.org/D11298