1163328 - Add a tuple class to MFBT

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Botond Ballo [:botond]]

Tuples are a very useful utility type. The C++11 standard library has them (std::tuple), but since we can't use that yet, I propose adding one to MFBT.

Comment 1

[Botond Ballo [:botond]]

Attachment: A minimal tuple class

Here's a minimal tuple class implementation. Apart from various forms of construction and assignment, the only operation it supports is index-based access to the tuple elements via a 'Get<N>(tuple)' non-member function (analogous to C++11's std::get). There's also a MakeTuple function for constructing a tuple (analogous to std::make_tuple).

I propose landing this minimal implementation for now. Other operations, such as comparisons and an std::tie equivalent, can be added later as needed.

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8603850 [details] [diff] [review]
A minimal tuple class

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

I admit to not going over this with a fine-toothed style comb or anything.

WDYT about building a non-recursive tuple implementation (or at least non-recursive get<>(), which is what I think libc++ does)?

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8603850 [details] [diff] [review]
A minimal tuple class

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

Resetting the review flag because Botond pointed out that I couldn't read. :)

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8603850 [details] [diff] [review]
A minimal tuple class

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

Some tests for this would be most welcome.

WDYT about implementing the moral equivalents of std::tuple_element and std::tuple_size?  (A follow-up bug for those would be fine.)

::: mfbt/Tuple.h
@@ +86,5 @@
> +  // Copy and move constructors.
> +  // We'd like to use '= default' to implement these, but that errors out
> +  // for types that are not copy-constructible like nsAutoPtr. (Members of the
> +  // C++ standards committee's Core Working Group who are sitting beside me as
> +  // I write this agree that this is a C++ language defect.)

Maybe rewriting this parenthetical comment to something like "This behavior will likely be changed in some future C++ standard, but for now, we have to manually implement these constructors."

@@ +222,5 @@
> +    -> decltype(Move(mozilla::Get<Index>(aTuple)))
> +{
> +  // We need a 'mozilla::' qualification here to avoid
> +  // name lookup only finding the current function.
> +  return Move(mozilla::Get<Index>(aTuple));

Stupid C++ name lookup questions, part N: this mozilla:: prefix then forces it to consider the non-const reference version for overloading?

Comment 5

[Botond Ballo [:botond]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #4)
> Some tests for this would be most welcome.

I'm glad you suggested this, because writing tests flushed out several bugs in the implementation :)

A particularly annoying one is that the addition of the converting constructor (the one that accepts objects convertible to the element types) broke copy construction from a non-const tuple, because in that case the type of the argument is a non-const lvalue reference to the tuple type, and the converting constructor becomes a better match then either the copy constructor (which takes a const lvalue reference) or the move constructor (which takes an rvalue reference). libstdc++ works around this by using enable_if to only enable the converting constructor when its arguments are actually convertible to the tuple elements. I think I'm going to have to do the same thing.

> WDYT about implementing the moral equivalents of std::tuple_element and
> std::tuple_size?  (A follow-up bug for those would be fine.)

Yeah, let's do those in a follow-up.

> @@ +222,5 @@
> > +    -> decltype(Move(mozilla::Get<Index>(aTuple)))
> > +{
> > +  // We need a 'mozilla::' qualification here to avoid
> > +  // name lookup only finding the current function.
> > +  return Move(mozilla::Get<Index>(aTuple));
> 
> Stupid C++ name lookup questions, part N: this mozilla:: prefix then forces
> it to consider the non-const reference version for overloading?

Yep.

Comment 6

[Botond Ballo [:botond]]

Attachment: Part 1 - Add an And<...> class to TypeTraits.h

This utility is needed to do the convertibility check described in the previous comment.

Comment 7

[Botond Ballo [:botond]]

Attachment: Part 2 - A minimal tuple class

I think the changes are non-trivial enough to warrant another review.

Comment 8

[Botond Ballo [:botond]]

Attachment: Part 3 - Tests for Tuple

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8607884 [details] [diff] [review]
Part 1 - Add an And<...> class to TypeTraits.h

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

I think this is reasonable, except that AFAICS--and maybe I'm looking at the wrong sources--there's no std::and template, and we've tried to keep TypeTraits.h "in sync" with <type_traits>.

I see a couple of different options:

1. Move And into mozilla::detail in TypeTraits.h, and use that.
2. Move And into a different header:
   a. Its own header;
   b. Tuple.h, since that's the only thing that uses it;
   c. TemplateLib.h, since it seems potentially useful elsewhere and "mozilla/And.h" is not a great name for a header.

I like 2b or 2c, personally.  WDYT?

Comment 10

[Nathan Froyd [:froydnj]]

Comment on attachment 8607889 [details] [diff] [review]
Part 2 - A minimal tuple class

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

Apologies for the delay in reviewing this.

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8607890 [details] [diff] [review]
Part 3 - Tests for Tuple

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

r=me with the changes below.

::: mfbt/tests/TestTuple.cpp
@@ +24,5 @@
> +
> +#define CHECK(c) \
> +  do { \
> +    bool cond = !!(c); \
> +    MOZ_ASSERT(cond, "Failed assertion: " #c); \

Two things:

1) Please make this MOZ_RELEASE_ASSERT so our release builds and our debug builds test the same thing.
2) Why have test functions that return booleans when MOZ_RELEASE_ASSERT will crash the test if things fail?  Seems better to just have Test* functions return |void| and let MOZ_RELEASE_ASSERT handle failure.

(Or did you make the CHECK functions to handle -Wunused-variable warnings?  MOZ_RELEASE_ASSERT will take care of that too.)

Comment 12

[Botond Ballo [:botond]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #11)
> (Or did you make the CHECK functions to handle -Wunused-variable warnings? 
> MOZ_RELEASE_ASSERT will take care of that too.)

I actually just copied the CHECK macro verbatim from TestUniquePtr.cpp ;)

Comment 13

[Botond Ballo [:botond]]

Attachment: Part 1 - Add an And<...> class to TypeTraits.h

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #9)
> AFAICS--and maybe I'm looking at the
> wrong sources--there's no std::and template

You're right, there is no std::and. (Little-known trivia: there also never can be, because 'and' is a keyword, with the same meaning as the operator '&&'.)

> , and we've tried to keep
> TypeTraits.h "in sync" with <type_traits>.
> 
> I see a couple of different options:
> 
> 1. Move And into mozilla::detail in TypeTraits.h, and use that.
> 2. Move And into a different header:
>    a. Its own header;
>    b. Tuple.h, since that's the only thing that uses it;
>    c. TemplateLib.h, since it seems potentially useful elsewhere and
> "mozilla/And.h" is not a great name for a header.
> 
> I like 2b or 2c, personally.  WDYT?

I went with 2c.

(Aside: I spent about 20 minutes debugging why 't1::And' wasn't compiling...)

Comment 14

[Botond Ballo [:botond]]

Attachment: Part 2 - A minimal tuple class

Updated to use tl::And from TemplateLib.h. Carrying r+.

Comment 15

[Botond Ballo [:botond]]

Attachment: Part 3 - Tests for Tuple

Made requested changes to the CHECK macro. Carrying r+.

Comment 16

[Nathan Froyd [:froydnj]]

Comment on attachment 8609155 [details] [diff] [review]
Part 1 - Add an And<...> class to TypeTraits.h

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

r=me with the commit message fixed up to point at the correct header.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0fe6e8656fc9
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e588075e7ee
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ea2a40f50f7

Comment 18

[Botond Ballo [:botond]]

(The Part 1 commit message is fixed in my push.)

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0fe6e8656fc9
https://hg.mozilla.org/mozilla-central/rev/1e588075e7ee
https://hg.mozilla.org/mozilla-central/rev/2ea2a40f50f7