[PATCH] D11272: Add a TrailingObjects template class.

[Richard Smith]

rsmith added a comment.

This looks like a good approach to me.


================
Comment at: include/llvm/Support/TrailingObjects.h:41-42
@@ +40,4 @@
+//
+// TODO: Use a variadic template instead of multiple copies of the
+// TrailingObjects class?
+//
----------------
That sounds like a cleaner interface to me, even if you only implement it for the N = 1 and N  = 2 cases.

================
Comment at: include/llvm/Support/TrailingObjects.h:76
@@ +75,3 @@
+  // function is instantiated.
+  static void verifyTrailingObjectsAlignment() {
+    static_assert(llvm::AlignOf<BaseTy>::Alignment >=
----------------
Have you considered putting a `static_assert` in here for `std::is_final<BaseTy>()`? (We'd need to add the `final` to all the classes where we use this, but that's easier to do when rolling it out than after the fact.)

================
Comment at: include/llvm/Support/TrailingObjects.h:115
@@ +114,3 @@
+  // array may have zero or more elements in it.
+  template <typename T> const T *getTrailingObjects() const {
+    verifyTrailingObjectsAlignment();
----------------
I think it'd be useful to also provide an accessor that returns an `ArrayRef<T>`. Perhaps `ArrayRef<T> trailing<T>()` / `const T *trailing_begin<T>()` / `const T *trailing_end<T>()`? (Though maybe the longer names are more appropriate for an internal interface.)

================
Comment at: lib/IR/AttributeImpl.h:201-202
@@ -201,1 +200,4 @@
 
+  // Helper fn for TrailingObjects class.
+  size_t numTrailingObjects(OverloadToken<IndexAttrPair>) { return NumAttrs; }
+
----------------
Do you need this? It looks like you only ever grab the beginning of the trailing array from the base class.


http://reviews.llvm.org/D11272