Add an implementation of std::initializer_list

RESOLVED FIXED in Firefox 46

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jrmuizel, Assigned: jrmuizel)

Tracking

unspecified
mozilla46
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(4 attachments, 5 obsolete attachments)

(Assignee)

Description

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

Comment 1

3 years ago
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.
(Assignee)

Comment 2

3 years ago
|         #if __GLIBCXX__ >= 20090421 // GCC 4.4.0
|             #define HAS_CXX0X_UNORDERED_MAP
|             #define HAS_CXX0X_UNORDERED_SET
|             #define HAS_CXX0X_INITIALIZER_LIST
|         #endif
(Assignee)

Comment 4

3 years ago
Created attachment 8694731 [details] [diff] [review]
A working implementation
Attachment #8694523 - Attachment is obsolete: true
Attachment #8694731 - Flags: feedback?(botond)
(Assignee)

Comment 5

3 years ago
Created attachment 8694732 [details] [diff] [review]
Add initializer_list constructor to nsTArray
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
Attachment #8694731 - Flags: feedback?(botond) → feedback+
(Assignee)

Comment 7

3 years ago
Created attachment 8695445 [details] [diff] [review]
Add an implementation of std::initializer_list
Attachment #8694731 - Attachment is obsolete: true
Attachment #8695445 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 8

3 years ago
Created attachment 8695446 [details] [diff] [review]
Add initializer_list constructor to nsTArray
Attachment #8694732 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8695445 - Flags: review?(jwalden+bmo) → review?(nfroyd)
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
Attachment #8695445 - Flags: review?(nfroyd) → feedback+
(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)?
(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.
(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.
(Assignee)

Comment 13

3 years ago
Created attachment 8704315 [details] [diff] [review]
Add an implementation of std::initializer_list
Assignee: nobody → jmuizelaar
Attachment #8695445 - Attachment is obsolete: true
Attachment #8704315 - Flags: review?(nfroyd)
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.
Attachment #8704315 - Flags: review?(nfroyd) → review+

Comment 16

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bedd35d9ebe6
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
(Assignee)

Comment 17

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

This starts using the new initializer_list support we have.
Attachment #8695446 - Attachment is obsolete: true
Attachment #8705148 - Flags: review?(nfroyd)
Attachment #8705148 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 18

3 years ago
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.
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?
Flags: needinfo?(jmuizelaar)
(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
(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.
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?
Attachment #8706771 - Flags: review?(botond)
Assignee: jmuizelaar → bbirtles
Oops, bzexport re-assigned to me.
Assignee: bbirtles → jmuizelaar
Flags: needinfo?(jmuizelaar)
Attachment #8706771 - Flags: review?(botond) → review+
(Assignee)

Comment 24

3 years ago
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.
Attachment #8705808 - Flags: review?(mh+mozilla)
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.
Attachment #8705808 - Flags: review?(mh+mozilla) → review-
(Assignee)

Comment 28

3 years ago
(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+.
(Assignee)

Updated

3 years ago
Attachment #8705808 - Flags: review- → review?(mh+mozilla)
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.
Attachment #8705808 - Flags: review?(mh+mozilla) → review+
Depends on: 1245601
No longer depends on: 1245601
You need to log in before you can comment on or make changes to this bug.