[PATCH] D11272: Add a TrailingObjects template class.

[James Y Knight]

jyknight added inline comments.

================
Comment at: include/llvm/Support/TrailingObjects.h:41-42
@@ +40,4 @@
+//
+// TODO: Use a variadic template instead of multiple copies of the
+// TrailingObjects class?
+//
----------------
rsmith wrote:
> That sounds like a cleaner interface to me, even if you only implement it for the N = 1 and N  = 2 cases.
The only thing stopping me had been the thought of having to write the functions using recursive template expansions, which are typically not fun to write, nor to read.

But, just using a 1-type partial specialization to the same name is a good idea -- done.

================
Comment at: include/llvm/Support/TrailingObjects.h:76
@@ +75,3 @@
+  // function is instantiated.
+  static void verifyTrailingObjectsAlignment() {
+    static_assert(llvm::AlignOf<BaseTy>::Alignment >=
----------------
rsmith wrote:
> 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.)
Great idea!

std::is_final is c++14 only, but I added some #ifdefery to use it, or the gcc/clang __is_final(T) when available.

Only, I immediately find:

  template<size_t N>
  class FixedSizeTemplateParameterList : public TemplateParameterList {
    NamedDecl *Params[N];

Gross. :)

================
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();
----------------
rsmith wrote:
> 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.)
That could be nice. I'd rather just replace the return type of getTrailingObjects<T> with a MutableArrayRef or ArrayRef than adding two more functions; callers that only want the pointer can just call .begin(). WDYT?

================
Comment at: lib/IR/AttributeImpl.h:201-202
@@ -201,1 +200,4 @@
 
+  // Helper fn for TrailingObjects class.
+  size_t numTrailingObjects(OverloadToken<IndexAttrPair>) { return NumAttrs; }
+
----------------
rsmith wrote:
> Do you need this? It looks like you only ever grab the beginning of the trailing array from the base class.
It's not strictly needed right now for the last trailing type (so: not at all for the 1-type variant, and only for the first type for the 2-type), as there are currently no helpers which return the total size.

But I included it, as I was expecting to want something like an objectSize, or the ArrayRef accessor you asked for above, at which point it'd be needed.


http://reviews.llvm.org/D11272