Page MenuHomePhabricator

[Feature] Add a builtin for indexing into parameter packs
ClosedPublic

Authored by ldionne on Dec 10 2015, 9:32 AM.

Details

Reviewers
majnemer
rsmith
Summary

This patch adds a __nth_element builtin that allows fetching the n-th type of a parameter pack with very little compile-time overhead. The patch was inspired by r252036 and r252115 by David Majnemer, which add a similar __make_integer_seq builtin for efficiently creating a std::integer_sequence.

Diff Detail

Event Timeline

ldionne updated this revision to Diff 42438.Dec 10 2015, 9:32 AM
ldionne retitled this revision from to [Feature] Add a builtin for indexing into parameter packs.
ldionne updated this object.
ldionne added a reviewer: majnemer.
ldionne added a subscriber: cfe-commits.

Hi Louis,

It would probably be useful to get the lines of context as well by doing a
full diff as describer here: http://llvm.org/docs/Phabricator.html#id3

For example, git diff -U999999 <branch-name>

ldionne updated this revision to Diff 42443.Dec 10 2015, 10:00 AM

Added full lines of context. Sorry for not doing it the first time around.

We should probably consider changing the name from __nth_element to something else, perhaps something that contains the word "pack". This is especially true if we are to implement other intrinsics for manipulating parameter packs, like slicing (as proposed by Eric Fiselier). Unless someone with a clearer view of the project suggests something, I would probably go with __pack_element (and then we could add __pack_slice, etc...). I have no strong preference.

This patch also raises the question of whether we should have a more separated way of adding builtin templates. My job here was very easy because I just copied David's revision and tweaked a couple of things here and there, but I could imagine it not being obvious at all otherwise. Also, and most importantly, if we add other builtin templates, do we want these builtins to "clutter" the rest of the code instead of being somewhat isolated? I don't know the answer to these questions, but I'd just like to point them out.

lib/AST/DeclTemplate.cpp
1244–1248

It would be simpler and better (IMO) to have template <std::size_t Index, typename ...T> instead of template <typename IndexType, IndexType Index, typename ...T>, but I couldn't figure out how to create a non-type template parameter. Hence, I just adapted the code from David's revision for __make_integer_seq.

lib/Sema/SemaTemplate.cpp
2115–2116

More generally, what is the proper way to retrieve a non-type template argument as a number that I can then use as a normal std::size_t (or similar) in the code? I really just want to retrieve the index, which is given as a non-type template parameter to __nth_element, and convert it to a numeric index n in order to fetch the n-th element of the parameter pack.

ldionne updated this revision to Diff 44779.Jan 13 2016, 12:29 PM
ldionne added a reviewer: rsmith.

Rebase on top of the master branch, and add Richard Smith as a reviewer, since he also reviewed David Majnemer's original patch (suggested by Nathan Wilson).

rsmith added inline comments.Jan 13 2016, 1:43 PM
include/clang/Basic/Builtins.h
222–226

Please put the comma after BTK__make_integer_seq, not on this line.

lib/AST/DeclTemplate.cpp
1243

Rename this createNthElementParameterList.

1244–1248

Use NonTypeTemplateParmDecl::Create. You can get size_t from ASTContext::getSizeType.

lib/Sema/SemaTemplate.cpp
2115–2116

getExtValue is OK here, as we don't support size_t being larger than 64 bits.

rsmith edited edge metadata.Jan 13 2016, 1:45 PM

Bikeshedding on the name a bit... how about __type_pack_element?

Bikeshedding on the name a bit... how about __type_pack_element?

Hmm, I kind of felt like having nth in there implied we're indexing into something... What about __nth_pack_element?

Bikeshedding on the name a bit... how about __type_pack_element?

Hmm, I kind of felt like having nth in there implied we're indexing into something... What about __nth_pack_element?

If anything, I would probably start the name with __pack. This way, we could add other builtins that follow the same pattern, like __pack_slice, __pack_reverse and probably others.

Or if we don't want to start with __pack, then no problem, but we might want to settle on a prefix that will work for other builtins on packs as well.

ldionne updated this revision to Diff 44907.Jan 14 2016, 11:10 AM
ldionne edited edge metadata.
ldionne marked 4 inline comments as done.

Address Richard Smith's review comments:

  • Remove IndexType parameter, and make Index a std::size_t
  • Remove assertion about APSInt::GetExtValue()
  • Other style changes

Still left to do:

  • Settle on a name for __nth_element
  • Should we produce a diagnostic when the index is out of bounds, or is this reserved for standard-mandated diagnostics (and we should use another mean to report the error)?
ldionne updated this revision to Diff 48012.Feb 15 2016, 2:09 PM

Address some remaining issues:

  • Rename nth_element to type_pack_element, as suggested by Richard Smith
  • Fix template parameter position issue in createTypePackElementParameterList

AFAICT, the only remaining issue is whether we should produce a diagnostic when the index is out of bounds, or whether this is reserved for standard diagnostics and we should use another mean to report the error. This is a trivial issue and I'll fix it or remove the TODO as soon as someone tells me the right way to go.

I'm really hoping that we can move forward with this patch.

rsmith accepted this revision.May 19 2016, 6:43 PM
rsmith edited edge metadata.

Sorry for the delay, this all LGTM. Do you need someone to commit it for you?

lib/Sema/SemaTemplate.cpp
2095–2098

This is fine; we get to specify how our extensions work, so we can say that an Index that's out of bounds is ill-formed.

This revision is now accepted and ready to land.May 19 2016, 6:43 PM

No problem for the delay; we're all busy :-). Unfortunately, the patch does not apply cleanly on master anymore, so I'll rebase it and someone can then commit it for me.

ldionne updated this revision to Diff 58619.May 26 2016, 8:45 AM
ldionne edited edge metadata.

Rebase the patch on top of the current master. The patch passes all of Clang's tests (make check-clang). I also removed the TODO comment about diagnostics, since that was answered by Richard Smith.

EricWF added a subscriber: EricWF.Jun 30 2016, 4:00 PM

Ping? Is there a reason this hasn't been committed?

EricWF closed this revision.Jun 30 2016, 6:31 PM

Committed in r274316.