Add FixedSizeStorage to TrailingObjects; NFC

[hubert.reinterpretcast]

Summary

This change introduces two types, FixedSizeStorage and FixedSizeStorageOwner, which can be used to provide stack-allocated objects with trailing objects.

[hubert.reinterpretcast]

An example of using FixedSizeStorage can be found in D19771.

include/llvm/Support/TrailingObjects.h

352

I named the member, _, as such, because I found that having a longer or more meaningful name (such as with_counts) here causes "noise" at the point where the type is used.

[hubert.reinterpretcast]

Use AlignOf<BaseTy> instead of AlignmentCalcHelper

Since BaseTy should be aligned properly for its trailing objects, the
use of AlignmentCalcHelper is unnecessary.

[faisalv]

Thanks for running with this! =)

include/llvm/Support/TrailingObjects.h

352

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...)>;

359

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);

[hubert.reinterpretcast]

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.

include/llvm/Support/TrailingObjects.h

352

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.

359

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.

[faisalv]

Thanks for your response. I don't feel strongly about either of those issues - especially since it seems you considered the options - and have good reasons for choosing the alternative.

I would also favor adding in a comment an example of how users are expected to use this functionality to create such objects on the stack.

Thanks!

P.S. Since you weren't seeing my previous comments emailed to you from phabricator - could you let me know if this one is?

[hubert.reinterpretcast]

Replace alias template with struct and typedef member; add usage example

[hubert.reinterpretcast]

Responded to all comments.

[hubert.reinterpretcast]

Ping.

[aaron.ballman]

include/llvm/Support/TrailingObjects.h

353

That's novel (we use it in two other places from what I can tell).

378

Should we have a const overload for this?

[hubert.reinterpretcast]

include/llvm/Support/TrailingObjects.h

353

This is a documented Doxygen command:
https://www.stack.nl/~dimitri/doxygen/manual/commands.html#cmdcode

The presence of the file extension in the existing use is mostly harmless to older versions of Doxygen:
http://www.llvm.org/docs/doxygen/html/classllvm_1_1TypeBuilder.html#details

Since this is present in a .h header, I am not sure we will get C++ (as opposed to C) syntax coloring reliably otherwise.

378

I can add one. It will return a pointer-to-const since the semantics of this class is that it owns the memory.

[aaron.ballman]

I think this LGTM (with the const overload), but you should poke Richard to see what his thoughts are before committing.

include/llvm/Support/TrailingObjects.h

353

Huh, interesting. I'm not opposed to it, though I prefer consistency. I say leave it, though.

378

That's what I was expecting, so it sounds good to me.

[hubert.reinterpretcast]

Update to r273650 with the requested const accessor

[aaron.ballman]

Did Richard sign off on this off-line? Also, this has caused a bot failure:

7:57 AM <bb-chapuni> build #4390 of ninja-clang-i686-msc19-R is complete: Failure [failed build_clang_tools_1] Build details are at http://bb.pgr.jp/builders/ninja-clang-i686-msc19-R/builds/4390 blamelist: Hubert Tong <[e-mail]>

[hubert.reinterpretcast]

Reapply r273664 with workaround for MSVC

[hubert.reinterpretcast]

include/llvm/Support/TrailingObjects.h

363

@aaron.ballman; my attempt with an online MSVC compiler is that this works around the compiler error. Can you confirm that this patch is safe for your MSVC build?

Also, do you feel strongly about making Size private?

include/llvm/Support/TrailingObjects.h

public:
   /// object.
   template <typename... Tys>
   static LLVM_CONSTEXPR typename std::enable_if<
       std::is_same<Foo<TrailingTys...>, Foo<Tys...>>::value, size_t>::type
       totalSizeToAlloc(typename trailing_objects_internal::ExtractSecondType<
                        TrailingTys, size_t>::type... Counts) {
     return sizeof(BaseTy) + ParentType::additionalSizeToAllocImpl(0, Counts...);
   }
+
+  /// A type where its ::with_counts template member has a ::type member
+  /// suitable for use as uninitialized storage for an object with the given
+  /// trailing object counts. The template arguments are similar to those
+  /// of additionalSizeToAlloc.
+  ///
+  /// Use with FixedSizeStorageOwner, e.g.:
+  ///
+  /// \code{.cpp}
+  ///
+  /// MyObj::FixedSizeStorage<void *>::with_counts<1u>::type myStackObjStorage;
+  /// MyObj::FixedSizeStorageOwner
+  ///     myStackObjOwner(new ((void *)&myStackObjStorage) MyObj);
+  /// MyObj *const myStackObjPtr = myStackObjOwner.get();
+  ///
+  /// \endcode
+  template <typename... Tys> struct FixedSizeStorage {
+    template <size_t... Counts> struct with_counts {
+      enum { Size = totalSizeToAlloc<Tys...>(Counts...) };
+      typedef llvm::AlignedCharArray<
+          llvm::AlignOf<BaseTy>::Alignment, Size
+          > type;
+    };
+  };
+
+  /// A type that acts as the owner for an object placed into fixed storage.
+  class FixedSizeStorageOwner {
+  public:
+    FixedSizeStorageOwner(BaseTy *p) : p(p) {}
+    ~FixedSizeStorageOwner() {
+      assert(p && "FixedSizeStorageOwner owns null?");
+      p->~BaseTy();
+    }
+
+    BaseTy *get() { return p; }
+    const BaseTy *get() const { return p; }
+
+  private:
+    FixedSizeStorageOwner(const FixedSizeStorageOwner &) = delete;
+    FixedSizeStorageOwner(FixedSizeStorageOwner &&) = delete;
+    FixedSizeStorageOwner &operator=(const FixedSizeStorageOwner &) = delete;
+    FixedSizeStorageOwner &operator=(FixedSizeStorageOwner &&) = delete;
+
+    BaseTy *const p;
+  };
 };
 
 } // end namespace llvm
 
 #endif