[prev in list] [next in list] [prev in thread] [next in thread] 

List:       cfe-commits
Subject:    Re: [PATCH] D11298: Convert a few classes over to use the new TrailingObjects helper.
From:       James Y Knight <jyknight () google ! com>
Date:       2015-08-05 17:08:14
Message-ID: 103b8b870be5c6b4f61144f14e6498c5 () localhost ! localdomain
[Download RAW message or body]

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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[prev in list] [next in list] [prev in thread] [next in thread]