Closed Bug 1163329 Opened 5 years ago Closed 5 years ago

Add a utility for expanding a tuple into a variadic argument list to MFBT

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: botond, Assigned: botond)

References

Details

Attachments

(1 file, 1 obsolete file)

I filed bug 1163328 for adding a tuple class to MFBT.

To fully leverage the power of a tuple class with variadic templates, we need a way to expand the elements of a tuple into a variadic argument list.

The C++11 standard library provides such a mechanism (std::index_sequence, see [1]), but as we can't use that yet, I propose we add one to MFBT.

[1] http://en.cppreference.com/w/cpp/utility/integer_sequence
Attached patch Attempt at an implementation (obsolete) — Splinter Review
Here's an attempt at implementing this.

Naturally, it gives an internal compiler error on MSVC :/

I'll have to try playing with it locally on a Windows machine to try to work around it.
Assignee: nobody → botond
This is a much simplified implementation (for example, it only implements the equivalent of std::index_sequence rather than of the more general std::integer_sequence) that still satisfies the use case we want (expanding a tuple into a variadic argument list). The simplifications were necessary to get MSVC 2013 to swallow it.
Attachment #8603851 - Attachment is obsolete: true
Attachment #8607891 - Flags: review?(nfroyd)
I should mention that I didn't write tests for this because I felt it was effectively tested by its use in bug 1163320, but if you prefer having explicit tests for it, I can write some.
Comment on attachment 8607891 [details] [diff] [review]
A working implementation

Review of attachment 8607891 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/IntegerSequence.h
@@ +36,5 @@
> + *
> + * Solution:
> + *
> + *   Write a helper function which takes the tuple, and an index sequence
> + *   containing indices corresponding to the tuple indices.

I am mildly surprised that the standard doesn't define the |apply| helper, merely providing it as an example.  (It's possible I am looking at an old draft, though.)  Followup bug to provide TupleApply.h?
Attachment #8607891 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #4)
> ::: mfbt/IntegerSequence.h
> @@ +36,5 @@
> > + *
> > + * Solution:
> > + *
> > + *   Write a helper function which takes the tuple, and an index sequence
> > + *   containing indices corresponding to the tuple indices.
> 
> I am mildly surprised that the standard doesn't define the |apply| helper,
> merely providing it as an example.  (It's possible I am looking at an old
> draft, though.)  Followup bug to provide TupleApply.h?

There is an std::experimental::apply() in the Library Fundamentals TS [1].

We could provide this, but I'll note that to use it, you have to get the target function into a form that's callable as 'f(a1, ..., an)' where 'f' is the function and 'a1, ..., an' are the tuple elements. For example, the call site in nsRunnableMethodArguments in bug 1063320 passes the arguments to a member function via a member function pointer. To get that into the required form, I think we need something like std::bind or generic lambdas, neither of which we currently have.

[1] http://en.cppreference.com/w/cpp/experimental/apply
https://hg.mozilla.org/mozilla-central/rev/bf2c8ae05278
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.