Page MenuHomePhabricator

Add a TrailingObjects template class.
ClosedPublic

Authored by jyknight on Jul 16 2015, 11:06 AM.

Details

Summary

This is intended to help support the idiom of a class that has some
other types (or arrays of types) appended on the end, which is used
quite heavily in clang.

Diff Detail

Repository
rL LLVM

Event Timeline

jyknight updated this revision to Diff 29926.Jul 16 2015, 11:06 AM
jyknight retitled this revision from to Add a TrailingObjects template class..
jyknight updated this object.
jyknight added reviewers: bogner, majnemer, rsmith.
rsmith edited edge metadata.Jul 16 2015, 11:34 AM

This looks like a good approach to me.

include/llvm/Support/TrailingObjects.h
41–42 ↗(On Diff #29926)

That sounds like a cleaner interface to me, even if you only implement it for the N = 1 and N = 2 cases.

76 ↗(On Diff #29926)

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

115 ↗(On Diff #29926)

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

lib/IR/AttributeImpl.h
201–202 ↗(On Diff #29926)

Do you need this? It looks like you only ever grab the beginning of the trailing array from the base class.

silvas added a subscriber: silvas.Jul 16 2015, 2:15 PM
silvas added inline comments.
include/llvm/Support/TrailingObjects.h
16 ↗(On Diff #29926)

typo: arithemtic

jyknight added inline comments.Jul 17 2015, 8:52 AM
include/llvm/Support/TrailingObjects.h
41–42 ↗(On Diff #29926)

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.

76 ↗(On Diff #29926)

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

115 ↗(On Diff #29926)

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?

lib/IR/AttributeImpl.h
201–202 ↗(On Diff #29926)

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.

jyknight updated this revision to Diff 30009.Jul 17 2015, 9:22 AM
jyknight edited edge metadata.

Update for review comments

jyknight updated this revision to Diff 31242.Aug 3 2015, 9:10 AM

Switched to doxygen comment style, extracted the is_final as an
LLVM_IS_FINAL macro.

This revision was automatically updated to reflect the committed changes.