Closed
Bug 1228641
Opened 7 years ago
Closed 7 years ago
Add an implementation of std::initializer_list
Categories
(Core :: MFBT, defect)
Core
MFBT
Tracking
()
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: jrmuizel, Assigned: jrmuizel)
Details
Attachments
(4 files, 5 obsolete files)
4.98 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
5.66 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
1.07 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
1.26 KB,
patch
|
botond
:
review+
|
Details | Diff | Splinter Review |
This is needed for many uses of initializer lists. E.g. allowing nsTArrays to be constructed from an initializer list.
Comment 1•7 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•7 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 3•7 years ago
|
||
Assignee | ||
Comment 4•7 years ago
|
||
Attachment #8694523 -
Attachment is obsolete: true
Attachment #8694731 -
Flags: feedback?(botond)
Assignee | ||
Comment 5•7 years ago
|
||
Comment 6•7 years ago
|
||
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•7 years ago
|
||
Attachment #8694731 -
Attachment is obsolete: true
Attachment #8695445 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 8•7 years ago
|
||
Attachment #8694732 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8695445 -
Flags: review?(jwalden+bmo) → review?(nfroyd)
![]() |
||
Comment 9•7 years ago
|
||
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+
Comment 10•7 years ago
|
||
(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•7 years ago
|
||
(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•7 years ago
|
||
(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•7 years ago
|
||
Assignee: nobody → jmuizelaar
Attachment #8695445 -
Attachment is obsolete: true
Attachment #8704315 -
Flags: review?(nfroyd)
![]() |
||
Comment 14•7 years ago
|
||
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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bedd35d9ebe6
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Assignee | ||
Comment 17•7 years ago
|
||
This starts using the new initializer_list support we have.
Attachment #8695446 -
Attachment is obsolete: true
Attachment #8705148 -
Flags: review?(nfroyd)
![]() |
||
Updated•7 years ago
|
Attachment #8705148 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 18•7 years ago
|
||
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•6 years ago
|
||
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)
Comment 20•6 years ago
|
||
(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•6 years ago
|
||
(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•6 years ago
|
||
(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)
Updated•6 years ago
|
Assignee: jmuizelaar → bbirtles
Comment 23•6 years ago
|
||
Oops, bzexport re-assigned to me.
Assignee: bbirtles → jmuizelaar
Flags: needinfo?(jmuizelaar)
Updated•6 years ago
|
Attachment #8706771 -
Flags: review?(botond) → review+
Assignee | ||
Comment 24•6 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 26•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/888bce768ee8
Comment 27•6 years ago
|
||
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•6 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•6 years ago
|
Attachment #8705808 -
Flags: review- → review?(mh+mozilla)
Comment 29•6 years ago
|
||
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+
Comment 31•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6eb54d2362a0
Comment 33•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bf5934ed7c9c
You need to log in
before you can comment on or make changes to this bug.
Description
•