Closed Bug 1228641 Opened 9 years ago Closed 9 years ago

Add an implementation of std::initializer_list

Categories

(Core :: MFBT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: jrmuizel, Assigned: jrmuizel)

Details

Attachments

(4 files, 5 obsolete files)

This is needed for many uses of initializer lists. E.g. allowing nsTArrays to be constructed from an initializer list.
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.
| #if __GLIBCXX__ >= 20090421 // GCC 4.4.0 | #define HAS_CXX0X_UNORDERED_MAP | #define HAS_CXX0X_UNORDERED_SET | #define HAS_CXX0X_INITIALIZER_LIST | #endif
Attached patch patch (obsolete) (deleted) — Splinter Review
Attached patch A working implementation (obsolete) (deleted) — Splinter Review
Attachment #8694523 - Attachment is obsolete: true
Attachment #8694731 - Flags: feedback?(botond)
Attached patch Add initializer_list constructor to nsTArray (obsolete) (deleted) — Splinter Review
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+
Attached patch Add an implementation of std::initializer_list (obsolete) (deleted) — Splinter Review
Attachment #8694731 - Attachment is obsolete: true
Attachment #8695445 - Flags: review?(jwalden+bmo)
Attached patch Add initializer_list constructor to nsTArray (obsolete) (deleted) — Splinter Review
Attachment #8694732 - Attachment is obsolete: true
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: 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+
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
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+
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.
(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+
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-
(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+.
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.

Attachment

General

Created:
Updated:
Size: