1228641 - Add an implementation of std::initializer_list

Status: RESOLVED FIXED

Description

[Jeff Muizelaar [:jrmuizel]]

This is needed for many uses of initializer lists. E.g. allowing nsTArrays to be constructed from an initializer list.

Comment 1

[:Ehsan Akhgari]

The way to do this is roughly as follows:

* Figure out which compilers support __has_include.  clang and gcc>=5 should.  Then you can use it to figure out if <initializer_list> exists.
* For the remaining compilers, figure out which C++ libraries support std::initializer_list.  libcxx probably supports it on all interesting versions.  libstdc++ probably grew some support for it at some point.  Same with dinkumware.  stlport doesn't have it.  The macros in mfbt/Compiler.h can be helpful here.
* For the remaining compilers where this is unsupported, do our own version.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

|         #if __GLIBCXX__ >= 20090421 // GCC 4.4.0
|             #define HAS_CXX0X_UNORDERED_MAP
|             #define HAS_CXX0X_UNORDERED_SET
|             #define HAS_CXX0X_INITIALIZER_LIST
|         #endif

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Created attachment 8694523 [details] [diff] [review]
patch

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Created attachment 8694731 [details] [diff] [review]
A working implementation

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Created attachment 8694732 [details] [diff] [review]
Add initializer_list constructor to nsTArray

Comment 6

[Botond Ballo [:botond] [standards meeting Nov 5-10]]

Comment on attachment 8694731 [details] [diff] [review]
A working implementation

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

Thanks for working on this, Jeff!

::: mfbt/InitializerList.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + *  * License, v. 2.0. If a copy of the MPL was not distributed with this
> + *   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* A polyfill for initializer_list if it doesn't exist */

std::initializer_list

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Created attachment 8695445 [details] [diff] [review]
Add an implementation of std::initializer_list

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Created attachment 8695446 [details] [diff] [review]
Add initializer_list constructor to nsTArray

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8695445 [details] [diff] [review]
Add an implementation of std::initializer_list

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

This looks reasonable; I think some comments on the implementation would be helpful.  I don't think we need a massive comment describing use like UniquePtr.h has, because this class isn't really designed to be instantiated directly by end users.

This needs a short and sweet test, too.

::: mfbt/InitializerList.h
@@ +14,5 @@
> +#  define MOZ_HAVE_INITIALIZER_LIST
> +#elif MOZ_USING_LIBSTDCXX && __GLIBCXX__ >= 20090421 // GCC 4.4.0
> +#  define MOZ_HAVE_INITIALIZER_LIST
> +#elif _MSC_VER > 1700
> +#  define MOZ_HAVE_INITIALIZER_LIST

We can make this unconditionally #elif defined(_MSC_VER), because we don't support 2012 anymore, right?

@@ +23,5 @@
> +
> +#ifdef MOZ_HAVE_INITIALIZER_LIST
> +#  include <initializer_list>
> +#else
> +namespace std

A short comment about how we would normally define things like this in mozilla::, but making everything work requires using std:: and its naming scheme, would be helpful.

@@ +30,5 @@
> +template<class T>
> +class initializer_list
> +{
> +    const T* mBegin;
> +    size_t   mSize;

This probably deserves a comment about how the only compiler(s) we care about here are gcc and clang, and this is the representation they use.  (MSVC uses a pair-of-pointers representation, FWIW.)

@@ +32,5 @@
> +{
> +    const T* mBegin;
> +    size_t   mSize;
> +
> +    initializer_list(const T* begin, size_t size) : mBegin(begin), mSize(size) {}

This constructor needs some documentation on why it exists; libstdc++ says that the compiler can call this constructor directly.

@@ +35,5 @@
> +
> +    initializer_list(const T* begin, size_t size) : mBegin(begin), mSize(size) {}
> +public:
> +
> +    initializer_list() : mBegin(nullptr), mSize(0) {}

This should be MOZ_CONSTEXPR.  Ideally, the member functions below would be MOZ_CONSTEXPR as well, but I'm not sure if that would end well...

@@ +39,5 @@
> +    initializer_list() : mBegin(nullptr), mSize(0) {}
> +
> +    typedef T value_type;
> +    typedef const T& reference;
> +    typedef const T& const_referenece;

|const_reference|, please.

@@ +46,5 @@
> +    typedef const T* const_iterator;
> +
> +    size_t size() { return mSize; }
> +    const T* begin() { return mBegin; }
> +    const T* end() { return mBegin + mSize; }

These member functions are defined to be const; please change them.

@@ +49,5 @@
> +    const T* begin() { return mBegin; }
> +    const T* end() { return mBegin + mSize; }
> +};
> +
> +}

} // namespace std

Comment 10

[Botond Ballo [:botond] [standards meeting Nov 5-10]]

(In reply to Nathan Froyd [:froydnj] from comment #9)
> @@ +30,5 @@
> > +template<class T>
> > +class initializer_list
> > +{
> > +    const T* mBegin;
> > +    size_t   mSize;
> 
> This probably deserves a comment about how the only compiler(s) we care
> about here are gcc and clang, and this is the representation they use. 
> (MSVC uses a pair-of-pointers representation, FWIW.)

I'm asking mostly out of curiosity, but do you think the compiler accesses the representation (as opposed to just the constructor)?

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to Botond Ballo [:botond] from comment #10)
> (In reply to Nathan Froyd [:froydnj] from comment #9)
> > > +    const T* mBegin;
> > > +    size_t   mSize;
> > 
> > This probably deserves a comment about how the only compiler(s) we care
> > about here are gcc and clang, and this is the representation they use. 
> > (MSVC uses a pair-of-pointers representation, FWIW.)
> 
> I'm asking mostly out of curiosity, but do you think the compiler accesses
> the representation (as opposed to just the constructor)?

Hm, that's a good point!  Ideally the compiler would just access the public member functions, wouldn't they?

I see from groveling through GCC that it does check the definition of the fields:

https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/cp/class.c;h=216a30141d486755a2479c6472dd72fc1475d45b;hb=HEAD#l6944

so it's not completely out of the question that the compiler might do unusual things here.

Comment 12

[Botond Ballo [:botond] [standards meeting Nov 5-10]]

(In reply to Nathan Froyd [:froydnj] from comment #11)
> I see from groveling through GCC that it does check the definition of the
> fields:
> 
> https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/cp/class.c;
> h=216a30141d486755a2479c6472dd72fc1475d45b;hb=HEAD#l6944
> 
> so it's not completely out of the question that the compiler might do
> unusual things here.

Ah, nice find! It stands to reason, then, that MSVC could potentially have a similar check, in which case trying to polyfill for both compilers with the same class might not work out :) Thankfully, we don't need to do polyfilling for MSVC so all should be well.

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Created attachment 8704315 [details] [diff] [review]
Add an implementation of std::initializer_list

Comment 14

[Nathan Froyd [:froydnj]]

Comment on attachment 8704315 [details] [diff] [review]
Add an implementation of std::initializer_list

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

r=me with the change below.  Maybe change the commit message to "Add a polyfill of std::initializer_list"?

::: mfbt/tests/TestInitializerList.cpp
@@ +18,5 @@
> +
> +int
> +main()
> +{
> +  MOZ_ASSERT(sum({1, 2, 3, 4, 5, 6}) == 7 * 3);

MOZ_RELEASE_ASSERT, please, so the test doesn't completely disappear in non-debug builds.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bedd35d9ebe6

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bedd35d9ebe6

Comment 17

[Jeff Muizelaar [:jrmuizel]]

Created attachment 8705148 [details] [diff] [review]
Add initializer_list constructor to nsTArray

This starts using the new initializer_list support we have.

Comment 18

[Jeff Muizelaar [:jrmuizel]]

Created attachment 8705808 [details] [diff] [review]
Remove initializer_list from stl-headers

Including new before initializer_list seems to cause problems like:

c:\tools\vs2013\vc\include\xutility(1278) : error C2065: 'initializer_list' : undeclared identifier
c:\tools\vs2013\vc\include\xutility(1278) : error C2065: '_Ilist' : undeclared identifier
c:\tools\vs2013\vc\include\xutility(1281) : error C2433: 'rbegin' : 'inline' not permitted on data declarations
c:\tools\vs2013\vc\include\xutility(1281) : error C2365: 'std::rbegin' : redefinition; previous definition was 'function'
c:\tools\vs2013\vc\include\xutility(1281) : error C2998: 'std::reverse_iterator<const _Elem*> std::rbegin' : cannot be a template definition
c:\tools\vs2013\vc\include\xutility(1284) : error C2065: 'initializer_list' : undeclared identifier
c:\tools\vs2013\vc\include\xutility(1284) : error C2065: '_Ilist' : undeclared identifier
c:\tools\vs2013\vc\include\xutility(1287) : error C2433: 'rend' : 'inline' not permitted on data declarations
c:\tools\vs2013\vc\include\xutility(1287) : error C2365: 'std::rend' : redefinition; previous definition was 'function'
c:\tools\vs2013\vc\include\xutility(1287) : error C2998: 'std::reverse_iterator<const _Elem*> std::rend' : cannot be a template definition 

initializer_list shouldn't very be doing any allocation or throwing exceptions so we should be fine.

Comment 19

[Brian Birtles (:birtles)]

On Android/B2G I get errors like:

/home/worker/objdir-gecko/objdir/dist/include/mozilla/InitializerList.h:43:51: error: declaration of 'size' shadows a member of 'this' [-Werror=shadow]
/home/worker/objdir-gecko/objdir/dist/include/mozilla/InitializerList.h:43:51: error: declaration of 'begin' shadows a member of 'this' [-Werror=shadow] 

(from: https://treeherder.mozilla.org/#/jobs?repo=try&revision=644cfbd9c693&selectedJob=15309839)

Am I doing it wrong?

Comment 20

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #19)
> (from:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=644cfbd9c693&selectedJob=15309839)

For reference, this is where I'm using it:
https://hg.mozilla.org/try/rev/397af02d3294#l3.92

Comment 21

[Botond Ballo [:botond] [standards meeting Nov 5-10]]

(In reply to Brian Birtles (:birtles) from comment #19)
> On Android/B2G I get errors like:
> 
> /home/worker/objdir-gecko/objdir/dist/include/mozilla/InitializerList.h:43:
> 51: error: declaration of 'size' shadows a member of 'this' [-Werror=shadow]
> /home/worker/objdir-gecko/objdir/dist/include/mozilla/InitializerList.h:43:
> 51: error: declaration of 'begin' shadows a member of 'this'
> [-Werror=shadow] 
> 
> Am I doing it wrong?

Nope, this platform just has a picky compiler. Changing the names of the parameters on the referenced line to |aBegin| and |aSize| should fix this.

Comment 22

[Brian Birtles (:birtles)]

Created attachment 8706771 [details] [diff] [review]
Rename begin/size to aBegin/aSize to avoid shadow warnings

(In reply to Botond Ballo [:botond] from comment #21)
> Nope, this platform just has a picky compiler. Changing the names of the
> parameters on the referenced line to |aBegin| and |aSize| should fix this.

Thanks! Something like this?

Comment 23

[Brian Birtles (:birtles)]

Oops, bzexport re-assigned to me.

Comment 24

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8705808 [details] [diff] [review]
Remove initializer_list from stl-headers

Ignore the commit message. It should be something more like:

Including new before initializer_list seems to cause problems like:

c:\tools\vs2013\vc\include\xutility(1278) : error C2065: 'initializer_list' : undeclared identifier
c:\tools\vs2013\vc\include\xutility(1278) : error C2065: '_Ilist' : undeclared identifier
c:\tools\vs2013\vc\include\xutility(1281) : error C2433: 'rbegin' : 'inline' not permitted on data declarations
c:\tools\vs2013\vc\include\xutility(1281) : error C2365: 'std::rbegin' : redefinition; previous definition was 'function'
c:\tools\vs2013\vc\include\xutility(1281) : error C2998: 'std::reverse_iterator<const _Elem*> std::rbegin' : cannot be a template definition
c:\tools\vs2013\vc\include\xutility(1284) : error C2065: 'initializer_list' : undeclared identifier
c:\tools\vs2013\vc\include\xutility(1284) : error C2065: '_Ilist' : undeclared identifier
c:\tools\vs2013\vc\include\xutility(1287) : error C2433: 'rend' : 'inline' not permitted on data declarations
c:\tools\vs2013\vc\include\xutility(1287) : error C2365: 'std::rend' : redefinition; previous definition was 'function'
c:\tools\vs2013\vc\include\xutility(1287) : error C2998: 'std::reverse_iterator<const _Elem*> std::rend' : cannot be a template definition 

initializer_list shouldn't very be doing any allocation or throwing exceptions so we should be fine.

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/888bce768ee8

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/888bce768ee8

Comment 27

[Mike Hommey [:glandium]]

Comment on attachment 8705808 [details] [diff] [review]
Remove initializer_list from stl-headers

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

You added it in the first patch, so just don't add it.

Comment 28

[Jeff Muizelaar [:jrmuizel]]

(In reply to Mike Hommey [:glandium] from comment #27)
> Comment on attachment 8705808 [details] [diff] [review]
> Remove initializer_list from stl-headers
> 
> Review of attachment 8705808 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You added it in the first patch, so just don't add it.

The first patch has already landed. Given you agree with removing it, I'll take this as an implicit r+.

Comment 29

[Mike Hommey [:glandium]]

Comment on attachment 8705808 [details] [diff] [review]
Remove initializer_list from stl-headers

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

You added it in the first patch, so just don't add it.

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6eb54d2362a0

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6eb54d2362a0

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf5934ed7c9c

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bf5934ed7c9c