James Y Knight <jykni...@google.com> writes: > jyknight updated this revision to Diff 31242. > jyknight added a comment. > > Switched to doxygen comment style, extracted the is_final as an > LLVM_IS_FINAL macro.
A couple more comments below, then this LGTM. > > http://reviews.llvm.org/D11272 > > Files: > include/llvm/Support/TrailingObjects.h > include/llvm/Support/type_traits.h > lib/IR/AttributeImpl.h > lib/IR/Attributes.cpp > unittests/Support/CMakeLists.txt > unittests/Support/TrailingObjectsTest.cpp > > Index: unittests/Support/TrailingObjectsTest.cpp > =================================================================== > --- /dev/null > +++ unittests/Support/TrailingObjectsTest.cpp > @@ -0,0 +1,147 @@ > +//=== - llvm/unittest/Support/TrailingObjectsTest.cpp > ---------------------===// > +// > +// The LLVM Compiler Infrastructure > +// > +// This file is distributed under the University of Illinois Open Source > +// License. See LICENSE.TXT for details. > +// > +//===----------------------------------------------------------------------===// > + > +#include "llvm/Support/TrailingObjects.h" > +#include "gtest/gtest.h" > + > +using namespace llvm; > + > +namespace { > +// This class, beyond being used by the test case, a nice > +// demonstration of the intended usage of TrailingObjects, with a > +// single trailing array. > +class Class1 final : TrailingObjects<Class1, short> { > + friend class TrailingObjects; > + > + unsigned NumShorts; > + > +protected: > + size_t numTrailingObjects(OverloadToken<short>) const { return NumShorts; } > + > + Class1(int *ShortArray, unsigned NumShorts) : NumShorts(NumShorts) { > + std::uninitialized_copy(ShortArray, ShortArray + NumShorts, > + getTrailingObjects<short>()); > + } > + > +public: > + static Class1 *create(int *ShortArray, unsigned NumShorts) { > + void *Mem = ::operator new(totalSizeToAlloc<short>(NumShorts)); > + return new (Mem) Class1(ShortArray, NumShorts); > + } > + > + short get(unsigned Num) const { return getTrailingObjects<short>()[Num]; } > + > + unsigned numShorts() const { return NumShorts; } > + > + // Pull some protected members in as public, for testability. > + using TrailingObjects::totalSizeToAlloc; > + using TrailingObjects::additionalSizeToAlloc; > + using TrailingObjects::getTrailingObjects; > +}; > + > +// Here, there are two singular optional object types appended. > +// Note that it fails to compile without the alignment spec. > +class LLVM_ALIGNAS(8) Class2 final : TrailingObjects<Class2, double, short> { > + friend class TrailingObjects; > + > + bool HasShort, HasDouble; > + > +protected: > + size_t numTrailingObjects(OverloadToken<short>) const { > + return HasShort ? 1 : 0; > + } > + size_t numTrailingObjects(OverloadToken<double>) const { > + return HasDouble ? 1 : 0; > + } > + > + Class2(bool HasShort, bool HasDouble) > + : HasShort(HasShort), HasDouble(HasDouble) {} > + > +public: > + static Class2 *create(short S = 0, double D = 0.0) { > + bool HasShort = S != 0; > + bool HasDouble = D != 0.0; > + > + void *Mem = > + ::operator new(totalSizeToAlloc<double, short>(HasDouble, HasShort)); > + Class2 *C = new (Mem) Class2(HasShort, HasDouble); > + if (HasShort) > + *C->getTrailingObjects<short>() = S; > + if (HasDouble) > + *C->getTrailingObjects<double>() = D; > + return C; > + } > + > + short getShort() const { > + if (!HasShort) > + return 0; > + return *getTrailingObjects<short>(); > + } > + > + double getDouble() const { > + if (!HasDouble) > + return 0.0; > + return *getTrailingObjects<double>(); > + } > + > + // Pull some protected members in as public, for testability. > + 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(Class1::totalSizeToAlloc<short>(1), sizeof(Class1) + > sizeof(short)); > + 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), 8u); // due to alignment > + > + 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((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)); > + > + EXPECT_EQ(C2->getDouble(), 4.2); > + EXPECT_EQ(C2->getShort(), 0); > + EXPECT_EQ(C2->getTrailingObjects<double>(), > + reinterpret_cast<double *>(C2 + 1)); > + EXPECT_EQ(C2->getTrailingObjects<short>(), > + reinterpret_cast<short *>(reinterpret_cast<double *>(C2 + 1) + > 1)); > + delete C1; > + delete C2; > +} > +} > Index: unittests/Support/CMakeLists.txt > =================================================================== > --- unittests/Support/CMakeLists.txt > +++ unittests/Support/CMakeLists.txt > @@ -41,6 +41,7 @@ > TargetRegistry.cpp > ThreadLocalTest.cpp > TimeValueTest.cpp > + TrailingObjectsTest.cpp > UnicodeTest.cpp > YAMLIOTest.cpp > YAMLParserTest.cpp > Index: lib/IR/Attributes.cpp > =================================================================== > --- lib/IR/Attributes.cpp > +++ lib/IR/Attributes.cpp > @@ -484,8 +484,7 @@ > // new one and insert it. > if (!PA) { > // Coallocate entries after the AttributeSetNode itself. > - void *Mem = ::operator new(sizeof(AttributeSetNode) + > - sizeof(Attribute) * SortedAttrs.size()); > + void *Mem = ::operator > new(totalSizeToAlloc<Attribute>(SortedAttrs.size())); > PA = new (Mem) AttributeSetNode(SortedAttrs); > pImpl->AttrsSetNodes.InsertNode(PA, InsertPoint); > } > @@ -617,9 +616,8 @@ > // create a new one and insert it. > if (!PA) { > // Coallocate entries after the AttributeSetImpl itself. > - void *Mem = ::operator new(sizeof(AttributeSetImpl) + > - sizeof(std::pair<unsigned, AttributeSetNode > *>) * > - Attrs.size()); > + void *Mem = ::operator new( > + AttributeSetImpl::totalSizeToAlloc<IndexAttrPair>(Attrs.size())); > PA = new (Mem) AttributeSetImpl(C, Attrs); > pImpl->AttrsLists.InsertNode(PA, InsertPoint); > } > @@ -736,9 +734,8 @@ > if (!AS) continue; > SmallVector<std::pair<unsigned, AttributeSetNode *>, 8>::iterator > ANVI = AttrNodeVec.begin(), ANVE; > - for (const AttributeSetImpl::IndexAttrPair > - *AI = AS->getNode(0), > - *AE = AS->getNode(AS->getNumAttributes()); > + for (const IndexAttrPair *AI = AS->getNode(0), > + *AE = AS->getNode(AS->getNumAttributes()); > AI != AE; ++AI) { > ANVE = AttrNodeVec.end(); > while (ANVI != ANVE && ANVI->first <= AI->first) > Index: lib/IR/AttributeImpl.h > =================================================================== > --- lib/IR/AttributeImpl.h > +++ lib/IR/AttributeImpl.h > @@ -18,6 +18,7 @@ > > #include "llvm/ADT/FoldingSet.h" > #include "llvm/IR/Attributes.h" > +#include "llvm/Support/TrailingObjects.h" > #include <string> > > namespace llvm { > @@ -141,13 +142,14 @@ > /// \class > /// \brief This class represents a group of attributes that apply to one > /// element: function, return type, or parameter. > -class AttributeSetNode : public FoldingSetNode { > +class AttributeSetNode final > + : public FoldingSetNode, > + public TrailingObjects<AttributeSetNode, Attribute> { This should probably be private inheritance, shouldn't it? We don't need or want is-a here. > unsigned NumAttrs; ///< Number of attributes in this node. > > AttributeSetNode(ArrayRef<Attribute> Attrs) : NumAttrs(Attrs.size()) { > // There's memory after the node where we can store the entries in. > - std::copy(Attrs.begin(), Attrs.end(), > - reinterpret_cast<Attribute *>(this + 1)); > + std::copy(Attrs.begin(), Attrs.end(), getTrailingObjects<Attribute>()); > } > > // AttributesSetNode is uniqued, these should not be publicly available. > @@ -170,7 +172,7 @@ > std::string getAsString(bool InAttrGrp) const; > > typedef const Attribute *iterator; > - iterator begin() const { return reinterpret_cast<iterator>(this + 1); } > + iterator begin() const { return getTrailingObjects<Attribute>(); } > iterator end() const { return begin() + NumAttrs; } > > void Profile(FoldingSetNodeID &ID) const { > @@ -181,27 +183,28 @@ > AttrList[I].Profile(ID); > } > }; > -static_assert( > - AlignOf<AttributeSetNode>::Alignment >= AlignOf<Attribute>::Alignment, > - "Alignment is insufficient for objects appended to AttributeSetNode"); > > > //===----------------------------------------------------------------------===// > /// \class > /// \brief This class represents a set of attributes that apply to the > function, > /// return type, and parameters. > -class AttributeSetImpl : public FoldingSetNode { > - friend class AttributeSet; > +typedef std::pair<unsigned, AttributeSetNode *> IndexAttrPair; > > -public: > - typedef std::pair<unsigned, AttributeSetNode*> IndexAttrPair; > +class AttributeSetImpl final > + : public FoldingSetNode, > + public TrailingObjects<AttributeSetImpl, IndexAttrPair> { > + friend class AttributeSet; > > private: > LLVMContext &Context; > unsigned NumAttrs; ///< Number of entries in this set. > > + // Helper fn for TrailingObjects class. > + size_t numTrailingObjects(OverloadToken<IndexAttrPair>) { return NumAttrs; > } > + > /// \brief Return a pointer to the IndexAttrPair for the specified slot. > const IndexAttrPair *getNode(unsigned Slot) const { > - return reinterpret_cast<const IndexAttrPair *>(this + 1) + Slot; > + return getTrailingObjects<IndexAttrPair>() + Slot; > } > > // AttributesSet is uniqued, these should not be publicly available. > @@ -222,8 +225,7 @@ > } > #endif > // There's memory after the node where we can store the entries in. > - std::copy(Attrs.begin(), Attrs.end(), > - reinterpret_cast<IndexAttrPair *>(this + 1)); > + std::copy(Attrs.begin(), Attrs.end(), > getTrailingObjects<IndexAttrPair>()); > } > > /// \brief Get the context that created this AttributeSetImpl. > @@ -273,10 +275,6 @@ > > void dump() const; > }; > -static_assert( > - AlignOf<AttributeSetImpl>::Alignment >= > - AlignOf<AttributeSetImpl::IndexAttrPair>::Alignment, > - "Alignment is insufficient for objects appended to AttributeSetImpl"); > > } // end llvm namespace > > Index: include/llvm/Support/type_traits.h > =================================================================== > --- include/llvm/Support/type_traits.h > +++ include/llvm/Support/type_traits.h > @@ -93,6 +93,15 @@ > > } > > +// If the compiler supports detecting whether a class is final, define > +// an LLVM_IS_FINAL macro. If it cannot be defined properly, this > +// macro will be left undefined. > +#if __cplusplus >= 201402L > +#define LLVM_IS_FINAL(Ty) std::is_final<Ty>() > +#elif __has_feature(is_final) || LLVM_GNUC_PREREQ(4, 7, 0) > +#define LLVM_IS_FINAL(Ty) __is_final(Ty) > +#endif > + > #ifdef LLVM_DEFINED_HAS_FEATURE > #undef __has_feature > #endif > Index: include/llvm/Support/TrailingObjects.h > =================================================================== > --- /dev/null > +++ include/llvm/Support/TrailingObjects.h > @@ -0,0 +1,234 @@ > +//===--- TrailingObjects.h - Variable-length classes ------------*- C++ > -*-===// > +// > +// The LLVM Compiler Infrastructure > +// > +// This file is distributed under the University of Illinois Open Source > +// License. See LICENSE.TXT for details. > +// > +//===----------------------------------------------------------------------===// > +/// > +/// \file > +/// This header defines support for implementing classes that have > +/// some trailing object (or arrays of objects) appended to them. The > +/// main purpose is to make it obvious where this idiom is being used, > +/// and to make the usage more idiomatic and more difficult to get > +/// wrong. > +/// > +/// The TrailingObject template abstracts away the reinterpret_cast, > +/// pointer arithmetic, and size calculations used for the allocation > +/// and access of appended arrays of objects, as well as asserts that > +/// the alignment of the classes involved are appropriate for the > +/// usage. Additionally, it ensures that the base type is final -- > +/// deriving from a class that expects data appended immediately after > +/// it is typically not safe. > +/// > +/// Users are expected to derive from this template, and provide > +/// numTrailingObjects implementations for each trailing type, > +/// e.g. like this sample: > +/// > +/// \code > +/// class VarLengthObj : TrailingObjects<VarLengthObj, int, double> { > +/// friend class TrailingObjects; > +/// > +/// unsigned NumInts, NumDoubles; > +/// size_t numTrailingObjects(OverloadToken<int>) const { return NumInts; } > +/// size_t numTrailingObjects(OverloadToken<double>) const { > +/// return NumDoubles; > +/// } > +/// }; > +/// \endcode > +/// > +/// You can access the appended arrays via 'getTrailingObjects', and > +/// determine the size needed for allocation via > +/// 'additionalSizeToAlloc' and 'totalSizeToAlloc'. > +/// > +/// All the methods implemented by this class are protected -- they > +/// are intended for use by the implementation of the class, not as > +/// part of its interface. This won't be necessary if subclasses use private inheritance. > +/// > +//===----------------------------------------------------------------------===// > + > +#ifndef LLVM_SUPPORT_TRAILINGOBJECTS_H > +#define LLVM_SUPPORT_TRAILINGOBJECTS_H > + > +#include <new> > +#include <type_traits> > +#include "llvm/Support/AlignOf.h" > +#include "llvm/Support/type_traits.h" > + > +namespace llvm { > + > +/// The base class for TrailingObjects* classes. > +class TrailingObjectsBase { > +protected: > + /// OverloadToken's purpose is to allow specifying function overloads > + /// for different types, without actually taking the types as > + /// parameters. (Necessary because member function templates cannot > + /// be specialized, so overloads must be used instead of > + /// specialization.) > + template <typename T> struct OverloadToken {}; > +}; > + > +// Internally used to indicate that the user didn't supply this value, > +// so the explicit-specialization for fewer args will be used. > +class NoTrailingTypeArg {}; > + > +// TODO: Consider using a single variadic implementation instead of > +// multiple copies of the TrailingObjects template? [but, variadic > +// template recursive implementations are annoying...] > + > +/// This is the two-type version of the TrailingObjects template; see > +/// file docstring for details. > +template <typename BaseTy, typename TrailingTy1, > + typename TrailingTy2 = NoTrailingTypeArg> > +class TrailingObjects : public TrailingObjectsBase { > +private: > + // Contains static_assert statements for the alignment of the > + // types. Must not be at class-level, because BaseTy isn't complete > + // at class instantiation time, but will be by the time this > + // function is instantiated. > + static void verifyTrailingObjectsAssertions() { > + static_assert(llvm::AlignOf<BaseTy>::Alignment >= > + llvm::AlignOf<TrailingTy1>::Alignment, > + "TrailingTy1 requires more alignment than BaseTy > provides"); > + static_assert( > + llvm::AlignOf<TrailingTy1>::Alignment >= > + llvm::AlignOf<TrailingTy2>::Alignment, > + "TrailingTy2 requires more alignment than TrailingTy1 provides"); > + > +#ifdef LLVM_IS_FINAL > + static_assert(LLVM_IS_FINAL(BaseTy), "BaseTy must be final."); > +#endif > + } > + > + // The next four functions are internal helpers for getTrailingObjects. > + static const TrailingTy1 *getTrailingObjectsImpl(const BaseTy *Obj, > + > OverloadToken<TrailingTy1>) { > + return reinterpret_cast<const TrailingTy1 *>(Obj + 1); > + } > + > + static TrailingTy1 *getTrailingObjectsImpl(BaseTy *Obj, > + OverloadToken<TrailingTy1>) { > + return reinterpret_cast<TrailingTy1 *>(Obj + 1); > + } > + > + static const TrailingTy2 *getTrailingObjectsImpl(const BaseTy *Obj, > + > OverloadToken<TrailingTy2>) { > + return reinterpret_cast<const TrailingTy2 *>( > + getTrailingObjectsImpl(Obj, OverloadToken<TrailingTy1>()) + > + Obj->numTrailingObjects(OverloadToken<TrailingTy1>())); > + } > + > + static TrailingTy2 *getTrailingObjectsImpl(BaseTy *Obj, > + OverloadToken<TrailingTy2>) { > + return reinterpret_cast<TrailingTy2 *>( > + getTrailingObjectsImpl(Obj, OverloadToken<TrailingTy1>()) + > + Obj->numTrailingObjects(OverloadToken<TrailingTy1>())); > + } > + > +protected: > + /// Returns a pointer to the trailing object array of the given type > + /// (which must be one of those specified in the class template). The > + /// array may have zero or more elements in it. > + template <typename T> const T *getTrailingObjects() const { > + verifyTrailingObjectsAssertions(); > + // Forwards to an impl function with overloads, since member > + // function templates can't be specialized. > + return getTrailingObjectsImpl(static_cast<const BaseTy *>(this), > + OverloadToken<T>()); > + } > + > + /// Returns a pointer to the trailing object array of the given type > + /// (which must be one of those specified in the class template). The > + /// array may have zero or more elements in it. > + template <typename T> T *getTrailingObjects() { > + verifyTrailingObjectsAssertions(); > + // Forwards to an impl function with overloads, since member > + // function templates can't be specialized. > + return getTrailingObjectsImpl(static_cast<BaseTy *>(this), > + OverloadToken<T>()); > + } > + > + /// Returns the size of the trailing data, if an object were > + /// allocated with the given counts (The counts are in the same order > + /// as the template arguments). This does not include the size of the > + /// base object. The template arguments must be the same as those > + /// used in the class; they are supplied here redundantly only so > + /// that it's clear what the counts are counting in callers. > + template <typename Ty1, typename Ty2, > + typename std::enable_if<std::is_same<Ty1, TrailingTy1>::value && > + std::is_same<Ty2, > TrailingTy2>::value, > + int>::type = 0> > + static constexpr size_t additionalSizeToAlloc(size_t Count1, size_t > Count2) { > + return sizeof(TrailingTy1) * Count1 + sizeof(TrailingTy2) * Count2; > + } > + > + /// Returns the total size of an object if it were allocated with the > + /// given trailing object counts. This is the same as > + /// additionalSizeToAlloc, except it *does* include the size of the base > + /// object. > + template <typename Ty1, typename Ty2> > + static constexpr size_t totalSizeToAlloc(size_t Count1, size_t Count2) { > + return sizeof(BaseTy) + additionalSizeToAlloc<Ty1, Ty2>(Count1, Count2); > + } > +}; > + > +/// This is the one-type version of the TrailingObjects template. See > +/// the two-type version for more documentation. > +template <typename BaseTy, typename TrailingTy1> > +class TrailingObjects<BaseTy, TrailingTy1, NoTrailingTypeArg> > + : public TrailingObjectsBase { > +private: > + static void verifyTrailingObjectsAssertions() { > + static_assert(llvm::AlignOf<BaseTy>::Alignment >= > + llvm::AlignOf<TrailingTy1>::Alignment, > + "TrailingTy1 requires more alignment than BaseTy > provides"); > + > +#ifdef LLVM_IS_FINAL > + static_assert(LLVM_IS_FINAL(BaseTy), "BaseTy must be final."); > +#endif > + } > + > + static const TrailingTy1 *getTrailingObjectsImpl(const BaseTy *Obj, > + > OverloadToken<TrailingTy1>) { > + return reinterpret_cast<const TrailingTy1 *>(Obj + 1); > + } > + > + static TrailingTy1 *getTrailingObjectsImpl(BaseTy *Obj, > + OverloadToken<TrailingTy1>) { > + return reinterpret_cast<TrailingTy1 *>(Obj + 1); > + } > + > +protected: > + template <typename T> const T *getTrailingObjects() const { > + verifyTrailingObjectsAssertions(); > + return getTrailingObjectsImpl(static_cast<const BaseTy *>(this), > + OverloadToken<T>()); > + } > + > + template <typename T> T *getTrailingObjects() { > + verifyTrailingObjectsAssertions(); > + return getTrailingObjectsImpl(static_cast<BaseTy *>(this), > + OverloadToken<T>()); > + } > + > + template <typename Ty1, > + typename std::enable_if<std::is_same<Ty1, TrailingTy1>::value, > + int>::type = 0> > + static constexpr size_t additionalSizeToAlloc(size_t Count1) { > + return sizeof(TrailingTy1) * Count1; > + } > + > + template <typename Ty1> > + static constexpr size_t totalSizeToAlloc(size_t Count1) { > + return sizeof(BaseTy) + additionalSizeToAlloc<Ty1>(Count1); > + } > +}; > + > +} // end namespace llvm > + > +#ifdef LLVM_DEFINED_HAS_FEATURE > +#undef __has_feature > +#endif Remove this, it's left over from a previous version. > + > +#endif _______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits