rsmith added inline comments.

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

================
Comment at: include/clang/AST/Decl.h:3638-3642
@@ -3628,6 +3637,7 @@
 
   ImplicitParamDecl **getParams() const {
-    return reinterpret_cast<ImplicitParamDecl **>(
-             const_cast<CapturedDecl *>(this) + 1);
+    // FIXME: doesn't seem like it should be using a const_cast.
+    return const_cast<CapturedDecl *>(this)
+        ->getTrailingObjects<ImplicitParamDecl *>();
   }
 
----------------
The uses of this below appear to be const-correct; maybe add const and non-const overloads without the `const_cast`?

================
Comment at: include/clang/AST/Decl.h:3702
@@ -3691,2 +3701,3 @@
   friend class ASTDeclWriter;
+  friend class TrailingObjects;
 };
----------------
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;

================
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 *> {
----------------
Can you remove the LLVM_ALIGNAS?

================
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)];
----------------
This type is now underaligned.

================
Comment at: include/clang/AST/DeclTemplate.h:146
@@ -140,1 +145,3 @@
+template <size_t N> class FixedSizeTemplateParameterListStorage {
+  char Mem[TemplateParameterList::totalSizeToAlloc<NamedDecl *>(N)];
 
----------------
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.)

================
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;
 
----------------
Please check in the removal of the `Owned` flag separately.

================
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]);
   }
 
----------------
Wow, this is still pretty gross. Can we use `std::pair<QualType, TypeSourceInfo*>` as the trailing object type, or something morally equivalent?

================
Comment at: lib/AST/Decl.cpp:3129
@@ +3128,3 @@
+  void *Buffer = Context.Allocate(
+      totalSizeToAlloc<TemplateArgumentLoc, FunctionTemplateDecl *>(
+          TArgs.size(), Ts.size()));
----------------
Why do you need to specify the types here? Shouldn't they be implied by the type of the `TrailingObjects` base class?

================
Comment at: lib/AST/Decl.cpp:3896
@@ -3893,3 +3895,3 @@
                                    unsigned NumParams) {
-  return new (C, DC, NumParams * sizeof(ImplicitParamDecl *))
+  return new (C, DC, additionalSizeToAlloc<ImplicitParamDecl *>(NumParams))
       CapturedDecl(DC, NumParams);
----------------
Likewise here.


http://reviews.llvm.org/D11298




_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits