[PATCH] D19770: Add FixedSizeStorage to TrailingObjects; NFC

[Hubert Tong via cfe-commits]

hubert.reinterpretcast planned changes to this revision.
hubert.reinterpretcast added a comment.

The alias template hit a build-compiler bug: the substitution of the pack expansion of `Counts` does not work properly. I am investigating a replacement with a templated struct with a typedef member.


================
Comment at: include/llvm/Support/TrailingObjects.h:352
@@ +351,3 @@
+    template <size_t... Counts>
+    using _ =
+        llvm::AlignedCharArray<llvm::AlignOf<BaseTy>::Alignment,
----------------
faisalv wrote:
> FWIW - while I see your point - the 'underscore' as a name seems out-of-style - as compared to what i'm used to reading within the llvm/clang code-base.  Personally I wouldn't mind calling it 'Type' or 'Ty'.
> 
> Also what are your thoughts about instead of making it a struct (and providing a more generic solution), just making it a template alias, constrained on the enclosing TrailingTypes pack (i.e. specialized for only being able to provide an answer for the enclosing type)?
> For e.g - something along the lines of
> template<size_t ... Counts> using FixedSizeStorageType = ...totalSizeToAlloc<TrailingTypes...>(Counts...)>;
> 
> 
> 
It only provides an answer for the enclosing type: `totalSizeToAlloc` is constrained.
The reason for the extra layer is to replicate the redundant specification that `totalSizeToAlloc` has.

================
Comment at: include/llvm/Support/TrailingObjects.h:359
@@ +358,3 @@
+  class FixedSizeStorageOwner {
+  public:
+    FixedSizeStorageOwner(BaseTy *p) : p(p) {}
----------------
faisalv wrote:
> What are your thoughts on having this guy be templated on <size_t ... Counts> and actually contain the Storage as a data member itself (as opposed to requiring folks to declare it - as you do for your Fixed TPL patch), and hiding the placement new under a forwarding templated constructor that forwards to BaseTy's ctors?
> 
> I imagine folks could just use it then as [If BaseTy = TemplateParameterList]:
> TemplateParameterList::FixedSizeTrailingCounts<5> TPL(ctors-arguments);
> 
> 
> 
I much prefer not to introduce forwarding templated constructors (at least without introducing a tag-wart). The smart-pointer idiom is well understood, and I would not want to step into novel design territory in production code.


http://reviews.llvm.org/D19770