TrailingObjects::FixedSizeStorage constexpr fixes + tests

[hubert.reinterpretcast]

Summary

This change fixes issues with LLVM_CONSTEXPR functions and TrailingObjects::FixedSizeStorage. In particular, some of the functions marked LLVM_CONSTEXPR used by FixedSizeStorage were not implemented such that they evaluate successfully as part of a constant expression despite constant arguments.

This change also implements a more traditional template-meta path to accommodate MSVC, and adds unit tests for FixedSizeStorage.

Drive-by fix: the access control for member of TrailingObjectsImpl is tightened.

[aaron.ballman]

I don't suppose there's a way to test these changes, is there?

include/llvm/Support/MathExtras.h

682

Switch to use doxygen-style comments?

include/llvm/Support/TrailingObjects.h

150

Does this need to be public?

[hubert.reinterpretcast]

In D22668#499164, @aaron.ballman wrote:

I don't suppose there's a way to test these changes, is there?

It's a utility class (which is not even used yet). I am not aware of testing for the ADTs, etc. aside from using them internally. Perhaps I should mark the change as NFC?

include/llvm/Support/MathExtras.h

682

Okay. I'll need to evaluate the appearance of the FIXME in Doxygen.

include/llvm/Support/TrailingObjects.h

150

This is in an "Impl" class in an "internal" namespace, so I believe leaving it public is reasonable.

[aaron.ballman]

In D22668#500340, @hubert.reinterpretcast wrote:

In D22668#499164, @aaron.ballman wrote:

I don't suppose there's a way to test these changes, is there?

It's a utility class (which is not even used yet). I am not aware of testing for the ADTs, etc. aside from using them internally. Perhaps I should mark the change as NFC?

We typically test ADTs using unit tests. Check out the ADTTests project in llvm. I definitely would not mark this as NFC -- it has functional changes, they're just not in use yet. If you can devise some tests to add to the ADT unit tests, that would help build confidence that this functionality is ready to be used, and helps protect us against accidental regressions in behavior later. It also exercises these code paths on all the compilers we support, which is probably the most important bit since these changes are a reaction to compiler differences in constexpr support.

include/llvm/Support/TrailingObjects.h

150

Reasonable, sure, but the preference is always to hide implementation details when possible from the public interface.

[hubert.reinterpretcast]

Address review comments: add tests, access control, Doxygen

[hubert.reinterpretcast]

Avoid MSVC C4099: replace 'struct' with 'class'

[hubert.reinterpretcast]

Replace call to LLVM_CONSTEXPR function in constant expression context

[hubert.reinterpretcast]

Make FixedSizeStorageOwner accessible to test code

[hubert.reinterpretcast]

Make FixedSizeStorage accessible to test code

FixedSizeStorage and not FixedSizeStorageOwner is what the test code needs. Use alias template to avoid issues with MSVC.

[aaron.ballman]

LGTM, thank you!

include/llvm/Support/AlignOf.h

 template <typename T> constexpr unsigned AlignOf<T>::Alignment;
 #endif
 
 /// alignOf - A templated function that returns the minimum alignment of
 ///  of a type.  This provides no extra functionality beyond the AlignOf
 ///  class besides some cosmetic cleanliness.  Example usage:
 ///  alignOf<int>() returns the alignment of an int.
 template <typename T>
-inline unsigned alignOf() { return AlignOf<T>::Alignment; }
+LLVM_CONSTEXPR inline unsigned alignOf() { return AlignOf<T>::Alignment; }
 
 /// \struct AlignedCharArray
 /// \brief Helper for building an aligned character array type.
 ///
 /// This template is used to explicitly build up a collection of aligned
 /// character array types. We have to build these up using a macro and explicit
 /// specialization to cope with old versions of MSVC and GCC where only an
 /// integer literal can be used to specify an alignment constraint. Once built

include/llvm/Support/MathExtras.h

 ///   alignTo(~0LL, 8, 3) = 3
 ///   alignTo(321, 255, 42) = 552
 /// \endcode
 inline uint64_t alignTo(uint64_t Value, uint64_t Align, uint64_t Skew = 0) {
   Skew %= Align;
   return (Value + Align - 1 - Skew) / Align * Align + Skew;
 }
 
+/// Returns the next integer (mod 2**64) that is greater than or equal to
+/// \p Value and is a multiple of \c Align. \c Align must be non-zero.
+template <uint64_t Align>
+LLVM_CONSTEXPR inline uint64_t alignTo(uint64_t Value) {
+  static_assert(Align != 0u, "Align must be non-zero");
+  return (Value + Align - 1) / Align * Align;
+}
+
+/// \c alignTo for contexts where a constant expression is required.
+/// \sa alignTo
+///
+/// \todo FIXME: remove when \c LLVM_CONSTEXPR becomes really \c constexpr
+template <uint64_t Align>
+struct AlignTo {
+  static_assert(Align != 0u, "Align must be non-zero");
+  template <uint64_t Value>
+  struct from_value {
+    static const uint64_t value = (Value + Align - 1) / Align * Align;
+  };
+};
+
 /// Returns the largest uint64_t less than or equal to \p Value and is
 /// \p Skew mod \p Align. \p Align must be non-zero
 inline uint64_t alignDown(uint64_t Value, uint64_t Align, uint64_t Skew = 0) {
   Skew %= Align;
   return (Value - Skew) / Align * Align + Skew;
 }
 
 /// Returns the offset to the next integer (mod 2**64) that is greater than

include/llvm/Support/TrailingObjects.h

 // varargs. Each level of inheritance picks off a single trailing type
 // then recurses on the rest. The "Align", "BaseTy", and
 // "TopTrailingObj" arguments are passed through unchanged through the
 // recursion. "PrevTy" is, at each level, the type handled by the
 // level right above it.
 
 template <int Align, typename BaseTy, typename TopTrailingObj, typename PrevTy,
           typename... MoreTys>
-struct TrailingObjectsImpl {
+class TrailingObjectsImpl {
   // The main template definition is never used -- the two
   // specializations cover all possibilities.
 };
 
 template <int Align, typename BaseTy, typename TopTrailingObj, typename PrevTy,
           typename NextTy, typename... MoreTys>
-struct TrailingObjectsImpl<Align, BaseTy, TopTrailingObj, PrevTy, NextTy,
-                           MoreTys...>
+class TrailingObjectsImpl<Align, BaseTy, TopTrailingObj, PrevTy, NextTy,
     : public TrailingObjectsImpl<Align, BaseTy, TopTrailingObj, NextTy,
                                  MoreTys...> {
 
   typedef TrailingObjectsImpl<Align, BaseTy, TopTrailingObj, NextTy, MoreTys...>
       ParentType;
 
-  // Ensure the methods we inherit are not hidden.
-  using ParentType::getTrailingObjectsImpl;
-  using ParentType::additionalSizeToAllocImpl;
+  struct RequiresRealignment {
+    static const bool value =
+        llvm::AlignOf<PrevTy>::Alignment < llvm::AlignOf<NextTy>::Alignment;
+  };
 
   static LLVM_CONSTEXPR bool requiresRealignment() {
-    return llvm::AlignOf<PrevTy>::Alignment < llvm::AlignOf<NextTy>::Alignment;
+    return RequiresRealignment::value;
   }
 
+protected:
+  // Ensure the inherited getTrailingObjectsImpl is not hidden.
+  using ParentType::getTrailingObjectsImpl;
+
   // These two functions are helper functions for
   // TrailingObjects::getTrailingObjects. They recurse to the left --
   // the result for each type in the list of trailing types depends on
   // the result of calling the function on the type to the
   // left. However, the function for the type to the left is
   // implemented by a *subclass* of this class, so we invoke it via
   // the TopTrailingObj, which is, via the
   // curiously-recurring-template-pattern, the most-derived type in
protected:
   }
 
   // Helper function for TrailingObjects::additionalSizeToAlloc: this
   // function recurses to superclasses, each of which requires one
   // fewer size_t argument, and adds its own size.
   static LLVM_CONSTEXPR size_t additionalSizeToAllocImpl(
       size_t SizeSoFar, size_t Count1,
       typename ExtractSecondType<MoreTys, size_t>::type... MoreCounts) {
-    return additionalSizeToAllocImpl(
+    return ParentType::additionalSizeToAllocImpl(
         (requiresRealignment()
-             ? llvm::alignTo(SizeSoFar, llvm::alignOf<NextTy>())
+             ? llvm::alignTo<llvm::AlignOf<NextTy>::Alignment>(SizeSoFar)
              : SizeSoFar) +
             sizeof(NextTy) * Count1,
         MoreCounts...);
   }
+
+  // additionalSizeToAllocImpl for contexts where a constant expression is
+  // required.
+  // FIXME: remove when LLVM_CONSTEXPR becomes really constexpr
+  template <size_t SizeSoFar, size_t Count1, size_t... MoreCounts>
+  struct AdditionalSizeToAllocImpl {
+    static_assert(sizeof...(MoreTys) == sizeof...(MoreCounts),
+                  "Number of counts do not match number of types");
+    static const size_t value = ParentType::template AdditionalSizeToAllocImpl<
+        (RequiresRealignment::value
+             ? llvm::AlignTo<llvm::AlignOf<NextTy>::Alignment>::
+                   template from_value<SizeSoFar>::value
+             : SizeSoFar) +
+            sizeof(NextTy) * Count1,
+        MoreCounts...>::value;
+  };
 };
 
 // The base case of the TrailingObjectsImpl inheritance recursion,
 // when there's no more trailing types.
 template <int Align, typename BaseTy, typename TopTrailingObj, typename PrevTy>
-struct TrailingObjectsImpl<Align, BaseTy, TopTrailingObj, PrevTy>
+class TrailingObjectsImpl<Align, BaseTy, TopTrailingObj, PrevTy>
     : public TrailingObjectsAligner<Align> {
+protected:
   // This is a dummy method, only here so the "using" doesn't fail --
   // it will never be called, because this function recurses backwards
   // up the inheritance chain to subclasses.
   static void getTrailingObjectsImpl();
 
   static LLVM_CONSTEXPR size_t additionalSizeToAllocImpl(size_t SizeSoFar) {
     return SizeSoFar;
   }
 
+  // additionalSizeToAllocImpl for contexts where a constant expression is
+  // required.
+  // FIXME: remove when LLVM_CONSTEXPR becomes really constexpr
+  template <size_t SizeSoFar> struct AdditionalSizeToAllocImpl {
+    static const size_t value = SizeSoFar;
+  };
+
   template <bool CheckAlignment> static void verifyTrailingObjectsAlignment() {}
 };
 
 } // end namespace trailing_objects_internal
 
 // Finally, the main type defined in this file, the one intended for users...
 
 /// See the file comment for details on the usage of the
 /// TrailingObjects type.
 template <typename BaseTy, typename... TrailingTys>
 class TrailingObjects : private trailing_objects_internal::TrailingObjectsImpl<
                             trailing_objects_internal::AlignmentCalcHelper<
                                 TrailingTys...>::Alignment,
                             BaseTy, TrailingObjects<BaseTy, TrailingTys...>,
                             BaseTy, TrailingTys...> {
 
   template <int A, typename B, typename T, typename P, typename... M>
-  friend struct trailing_objects_internal::TrailingObjectsImpl;
+  friend class trailing_objects_internal::TrailingObjectsImpl;
 
   template <typename... Tys> class Foo {};
 
   typedef trailing_objects_internal::TrailingObjectsImpl<
       trailing_objects_internal::AlignmentCalcHelper<TrailingTys...>::Alignment,
       BaseTy, TrailingObjects<BaseTy, TrailingTys...>, BaseTy, TrailingTys...>
       ParentType;
   using TrailingObjectsBase = trailing_objects_internal::TrailingObjectsBase;
public:
   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...);
   }
 
+  // totalSizeToAlloc for contexts where a constant expression is required.
+  // FIXME: remove when LLVM_CONSTEXPR becomes really constexpr
+  template <typename... Tys> struct TotalSizeToAlloc {
+    static_assert(
+        std::is_same<Foo<TrailingTys...>, Foo<Tys...>>::value,
+        "Arguments to TotalSizeToAlloc do not match with TrailingObjects");
+    template <size_t... Counts> struct with_counts {
+      static_assert(sizeof...(TrailingTys) == sizeof...(Counts),
+                    "Number of counts do not match number of types");
+      static const size_t value =
+          sizeof(BaseTy) +
+          ParentType::template AdditionalSizeToAllocImpl<0, Counts...>::value;
+    };
+  };
+
   /// 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...) };
+      enum {
+        Size = TotalSizeToAlloc<Tys...>::template with_counts<Counts...>::value
+      };
       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 {

unittests/Support/TrailingObjectsTest.cpp

public:
   }
   void operator delete(void *p) { ::operator delete(p); }
 
   short get(unsigned Num) const { return getTrailingObjects<short>()[Num]; }
 
   unsigned numShorts() const { return NumShorts; }
 
   // Pull some protected members in as public, for testability.
+  template <typename... Ty>
+  using FixedSizeStorage = TrailingObjects::FixedSizeStorage<Ty...>;
+
   using TrailingObjects::totalSizeToAlloc;
   using TrailingObjects::additionalSizeToAlloc;
   using TrailingObjects::getTrailingObjects;
 };
 
 // Here, there are two singular optional object types appended.  Note
 // that the alignment of Class2 is automatically increased to account
 // for the alignment requirements of the trailing objects.
public:
 
   double getDouble() const {
     if (!HasDouble)
       return 0.0;
     return *getTrailingObjects<double>();
   }
 
   // Pull some protected members in as public, for testability.
+  template <typename... Ty>
+  using FixedSizeStorage = TrailingObjects::FixedSizeStorage<Ty...>;
+
   using TrailingObjects::totalSizeToAlloc;
   using TrailingObjects::additionalSizeToAlloc;
   using TrailingObjects::getTrailingObjects;
 };
 
 TEST(TrailingObjects, OneArg) {
   int arr[] = {1, 2, 3};
   Class1 *C = Class1::create(arr, 3);
   EXPECT_EQ(sizeof(Class1), sizeof(unsigned));
   EXPECT_EQ(Class1::additionalSizeToAlloc<short>(1), sizeof(short));
   EXPECT_EQ(Class1::additionalSizeToAlloc<short>(3), sizeof(short) * 3);
 
+  EXPECT_EQ(
+      llvm::alignOf<Class1>(),
+      llvm::alignOf<Class1::FixedSizeStorage<short>::with_counts<1>::type>());
+  EXPECT_EQ(sizeof(Class1::FixedSizeStorage<short>::with_counts<1>::type),
+            Class1::totalSizeToAlloc<short>(1));
   EXPECT_EQ(Class1::totalSizeToAlloc<short>(1), sizeof(Class1) + sizeof(short));
+
+  EXPECT_EQ(
+      llvm::alignOf<Class1>(),
+      llvm::alignOf<Class1::FixedSizeStorage<short>::with_counts<3>::type>());
+  EXPECT_EQ(sizeof(Class1::FixedSizeStorage<short>::with_counts<3>::type),
+            Class1::totalSizeToAlloc<short>(3));
   EXPECT_EQ(Class1::totalSizeToAlloc<short>(3),
             sizeof(Class1) + sizeof(short) * 3);
 
   EXPECT_EQ(C->getTrailingObjects<short>(), reinterpret_cast<short *>(C + 1));
   EXPECT_EQ(C->get(0), 1);
   EXPECT_EQ(C->get(2), 3);
   delete C;
 }
 
 TEST(TrailingObjects, TwoArg) {
   Class2 *C1 = Class2::create(4);
   Class2 *C2 = Class2::create(0, 4.2);
 
   EXPECT_EQ(sizeof(Class2),
             llvm::alignTo(sizeof(bool) * 2, llvm::alignOf<double>()));
   EXPECT_EQ(llvm::alignOf<Class2>(), llvm::alignOf<double>());
 
   EXPECT_EQ((Class2::additionalSizeToAlloc<double, short>(1, 0)),
             sizeof(double));
   EXPECT_EQ((Class2::additionalSizeToAlloc<double, short>(0, 1)),
             sizeof(short));
   EXPECT_EQ((Class2::additionalSizeToAlloc<double, short>(3, 1)),
             sizeof(double) * 3 + sizeof(short));
 
+  EXPECT_EQ(
+      llvm::alignOf<Class2>(),
+      (llvm::alignOf<
+          Class2::FixedSizeStorage<double, short>::with_counts<1, 1>::type>()));
+  EXPECT_EQ(
+      sizeof(Class2::FixedSizeStorage<double, short>::with_counts<1, 1>::type),
+      (Class2::totalSizeToAlloc<double, short>(1, 1)));
   EXPECT_EQ((Class2::totalSizeToAlloc<double, short>(1, 1)),
             sizeof(Class2) + sizeof(double) + sizeof(short));
 
   EXPECT_EQ(C1->getDouble(), 0);
   EXPECT_EQ(C1->getShort(), 4);
   EXPECT_EQ(C1->getTrailingObjects<double>(),
             reinterpret_cast<double *>(C1 + 1));
   EXPECT_EQ(C1->getTrailingObjects<short>(), reinterpret_cast<short *>(C1 + 1));
class Class3 final : public TrailingObjects<Class3, double, short, bool> {
   size_t numTrailingObjects(OverloadToken<double>) const { return 1; }
   size_t numTrailingObjects(OverloadToken<short>) const { return 1; }
 };
 
 TEST(TrailingObjects, ThreeArg) {
   EXPECT_EQ((Class3::additionalSizeToAlloc<double, short, bool>(1, 1, 3)),
             sizeof(double) + sizeof(short) + 3 * sizeof(bool));
   EXPECT_EQ(sizeof(Class3), llvm::alignTo(1, llvm::alignOf<double>()));
+
+  EXPECT_EQ(llvm::alignOf<Class3>(),
+            (llvm::alignOf<Class3::FixedSizeStorage<
+                 double, short, bool>::with_counts<1, 1, 3>::type>()));
+  EXPECT_EQ(sizeof(Class3::FixedSizeStorage<double, short,
+                                            bool>::with_counts<1, 1, 3>::type),
+            (Class3::totalSizeToAlloc<double, short, bool>(1, 1, 3)));
+
   std::unique_ptr<char[]> P(new char[1000]);
   Class3 *C = reinterpret_cast<Class3 *>(P.get());
   EXPECT_EQ(C->getTrailingObjects<double>(), reinterpret_cast<double *>(C + 1));
   EXPECT_EQ(C->getTrailingObjects<short>(),
             reinterpret_cast<short *>(reinterpret_cast<double *>(C + 1) + 1));
   EXPECT_EQ(
       C->getTrailingObjects<bool>(),
       reinterpret_cast<bool *>(
           reinterpret_cast<short *>(reinterpret_cast<double *>(C + 1) + 1) +
           1));
 }
 
 class Class4 final : public TrailingObjects<Class4, char, long> {
   friend TrailingObjects;
   size_t numTrailingObjects(OverloadToken<char>) const { return 1; }
 };
 
 TEST(TrailingObjects, Realignment) {
   EXPECT_EQ((Class4::additionalSizeToAlloc<char, long>(1, 1)),
             llvm::alignTo(sizeof(long) + 1, llvm::alignOf<long>()));
   EXPECT_EQ(sizeof(Class4), llvm::alignTo(1, llvm::alignOf<long>()));
+
+  EXPECT_EQ(
+      llvm::alignOf<Class4>(),
+      (llvm::alignOf<
+          Class4::FixedSizeStorage<char, long>::with_counts<1, 1>::type>()));
+  EXPECT_EQ(
+      sizeof(Class4::FixedSizeStorage<char, long>::with_counts<1, 1>::type),
+      (Class4::totalSizeToAlloc<char, long>(1, 1)));
+
   std::unique_ptr<char[]> P(new char[1000]);
   Class4 *C = reinterpret_cast<Class4 *>(P.get());
   EXPECT_EQ(C->getTrailingObjects<char>(), reinterpret_cast<char *>(C + 1));
   EXPECT_EQ(C->getTrailingObjects<long>(),
             reinterpret_cast<long *>(llvm::alignAddr(
                 reinterpret_cast<char *>(C + 1) + 1, llvm::alignOf<long>())));
 }
 }