Closed
Bug 782647
Opened 12 years ago
Closed 12 years ago
Move nullptr to MFBT
Categories
(Core :: MFBT, defect)
Core
MFBT
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: Ms2ger, Assigned: jrmuizel)
References
Details
Attachments
(2 files, 2 obsolete files)
And kill the copies in nscore.h and gfx/2d/2D.h
Reporter | ||
Updated•12 years ago
|
Assignee: nobody → Ms2ger
Reporter | ||
Comment 1•12 years ago
|
||
Attachment #652179 -
Flags: review?(jones.chris.g)
Comment 2•12 years ago
|
||
Comment on attachment 652179 [details] [diff] [review]
Patch v1
I'm going to steal this review. Looks great!
Attachment #652179 -
Flags: review?(jones.chris.g) → review+
Comment 3•12 years ago
|
||
Comment on attachment 652179 [details] [diff] [review]
Patch v1
Review of attachment 652179 [details] [diff] [review]:
-----------------------------------------------------------------
::: mfbt/Types.h
@@ +25,5 @@
> #include <stddef.h>
>
> +/*
> + * Expose C++11 nullptr if it isn't available. For C, fall back to longs.
> + */
While you're at it, why don't you fix this comment? We actually fall back to (void*)0 for C, not longs; and __null for gcc; and longs for everything else.
Reporter | ||
Comment 4•12 years ago
|
||
I'll just drop that; it's clear enough from the code.
Comment 5•12 years ago
|
||
Comment on attachment 652179 [details] [diff] [review]
Patch v1
I seem to recall there being "issues" with having mfbt headers depend on configure checks. Whether we care about those issues enough or not, I don't know. Adding an r?glandium since he remembers the reasons better than I do.
Style-wise, the #ifdef indentation should be two spaces; I actually have a little trouble reading it quickly the way it is now, without that larger/consistent indentation.
Attachment #652179 -
Flags: review?(mh+mozilla)
Comment 6•12 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> Comment on attachment 652179 [details] [diff] [review]
> Patch v1
>
> I seem to recall there being "issues" with having mfbt headers depend on
> configure checks. Whether we care about those issues enough or not, I don't
> know. Adding an r?glandium since he remembers the reasons better than I do.
The problem with using configure defines is that you add occasions for shooting yourself in the foot. In the present case, if you start using nullptr in js, you'd need to remember to add the nullptr check to js/src/configure.in. Likewise for comm-central.
Comment 7•12 years ago
|
||
Comment on attachment 652179 [details] [diff] [review]
Patch v1
Review of attachment 652179 [details] [diff] [review]:
-----------------------------------------------------------------
::: mfbt/Types.h
@@ +26,5 @@
>
> +/*
> + * Expose C++11 nullptr if it isn't available. For C, fall back to longs.
> + */
> +#ifndef HAVE_NULLPTR
By the way, this looks wrong. If the compiler does have nullptr, there is no fallback for C, where it won't have it.
Attachment #652179 -
Flags: review?(mh+mozilla) → review-
Comment 8•12 years ago
|
||
As said on IRC today, I think we should take out the C fallback case.
Comment 9•12 years ago
|
||
Comment on attachment 652179 [details] [diff] [review]
Patch v1
Review of attachment 652179 [details] [diff] [review]:
-----------------------------------------------------------------
::: mfbt/Types.h
@@ +25,5 @@
> #include <stddef.h>
>
> +/*
> + * Expose C++11 nullptr if it isn't available. For C, fall back to longs.
> + */
This is perhaps kind of overkill-ish, but I think it would be better if we put nullptr in its own mfbt/NullPtr.h header, separate from everything else. That would make it easier to remove the header (and all uses of it) in the future when C++11 is ubiquitous. Really I'm not particularly fond of the Types.h header to begin with; it's kind of a dumping ground for stuff, rather than a carefully curated logically coherent set of things -- shades of Util.h. I'd like to get rid of it eventually, moving the library method macros into something like APIMacros.h, and moving the type includes into users.
Comment 10•12 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #9)
> Comment on attachment 652179 [details] [diff] [review]
> Patch v1
>
> Review of attachment 652179 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: mfbt/Types.h
> @@ +25,5 @@
> > #include <stddef.h>
> >
> > +/*
> > + * Expose C++11 nullptr if it isn't available. For C, fall back to longs.
> > + */
>
> This is perhaps kind of overkill-ish, but I think it would be better if we
> put nullptr in its own mfbt/NullPtr.h header, separate from everything else.
> That would make it easier to remove the header (and all uses of it) in the
> future when C++11 is ubiquitous.
Wouldn't it make sense to put all the C++11 compatibility stuff in one place?
Comment 11•12 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #9)
> This is perhaps kind of overkill-ish, but I think it would be better if we
> put nullptr in its own mfbt/NullPtr.h header, separate from everything else.
> That would make it easier to remove the header (and all uses of it) in the
> future when C++11 is ubiquitous.
This logic applies to all C++11 features, right? Like MOZ_OVERRIDE, MOZ_FINAL, etc. Should each of those be in its own header file?
Reporter | ||
Updated•12 years ago
|
Assignee: Ms2ger → nobody
Comment 12•12 years ago
|
||
Can we please resist scope creeping this bug? We can look into moving all of the C++11 compatibility hacks into their own header, but I think that should definitely be done in its own bug, and I also doubt the practical value very much, as if the C++98 history teaches us anything, it will be years before we get to a point where C++11 is ubiquitous. :/
Comment 13•12 years ago
|
||
(In reply to :Aryeh Gregor from comment #11)
> This logic applies to all C++11 features, right? Like MOZ_OVERRIDE,
> MOZ_FINAL, etc. Should each of those be in its own header file?
Nope. Those match the name on the box: attributes. nullptr ain't an attribute. :-) (The typed enum stuff I would move elsewhere, to be sure.) But that is indeed all aside -- I'm only concerned about nullptr here, well in accord with comment 12.
Comment 14•12 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #13)
> (In reply to :Aryeh Gregor from comment #11)
> > This logic applies to all C++11 features, right? Like MOZ_OVERRIDE,
> > MOZ_FINAL, etc. Should each of those be in its own header file?
>
> Nope. Those match the name on the box: attributes. nullptr ain't an
> attribute. :-) (The typed enum stuff I would move elsewhere, to be sure.)
> But that is indeed all aside -- I'm only concerned about nullptr here, well
> in accord with comment 12.
OK, so if I write a patch to:
1. Remove the C fallback.
2. Move this cruft into a new NullPtr.h header.
3. #include NullPtr.h in nscore.h (so that we don't need to include that header all around the code base)
would that be expected? Sorry that I'm asking this in advance of writing the patch, but this bug is getting very close to get stuck between the different things that people want, and I'd like to know whether this solution will be acceptable before I write that patch (and if not, I'll probably go ahead and WONTFIX this since I can't see any way out of that situation. :/ )
Comment 15•12 years ago
|
||
That sounds cool.
Comment 16•12 years ago
|
||
Assignee: nobody → ehsan
Attachment #652179 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #657094 -
Flags: review?(jwalden+bmo)
Comment 18•12 years ago
|
||
Comment on attachment 657094 [details] [diff] [review]
Patch (v2)
Review of attachment 657094 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good to me. My understanding was that HAVE_NULLPTR means configury stuff which means you run into the configure issue mentioned here already, so I'll forward to glandium to either say this is okay or to suggest some sort of solution or whatever.
::: gfx/2d/Types.h
@@ +9,2 @@
> #include "mozilla/StandardInteger.h"
> +#include "mozilla/NullPtr.h"
Probably these should be alphabetized.
::: xpcom/base/nscore.h
@@ +24,5 @@
> * Incorporate the core NSPR data types which XPCOM uses.
> */
> #include "prtypes.h"
> #include "mozilla/StandardInteger.h"
> +#include "mozilla/NullPtr.h"
Put this before StandardInteger.h so it's alphabetical.
Attachment #657094 -
Flags: review?(mh+mozilla)
Attachment #657094 -
Flags: review?(jwalden+bmo)
Attachment #657094 -
Flags: review+
Comment 19•12 years ago
|
||
Hrm, I thought that there was no work-around for the configure dependency?
Comment 20•12 years ago
|
||
Try run for 4f85136e52e8 is complete.
Detailed breakdown of the results available here:
https://tbpl.mozilla.org/?tree=Try&rev=4f85136e52e8
Results (out of 16 total builds):
success: 14
failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-4f85136e52e8
Comment 21•12 years ago
|
||
(In reply to Ehsan Akhgari [:ehsan] from comment #19)
> Hrm, I thought that there was no work-around for the configure dependency?
The workaround is a pile of #ifdef, and considering we do have such piles for other stuff in MFBT, I'm inclined to say we should do the same.
Comment 22•12 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #21)
> (In reply to Ehsan Akhgari [:ehsan] from comment #19)
> > Hrm, I thought that there was no work-around for the configure dependency?
>
> The workaround is a pile of #ifdef, and considering we do have such piles
> for other stuff in MFBT, I'm inclined to say we should do the same.
OK, I'm lost again. Which work-around are you talking about, and what are the examples of other places in MFBT where we do this?
Note that what we want to test here is whether the compiler can compile something like |void* foo = nullptr;| without including any headers, etc (which is what the configure check does.)
Comment 23•12 years ago
|
||
(In reply to Ehsan Akhgari [:ehsan] from comment #22)
> (In reply to Mike Hommey [:glandium] from comment #21)
> > (In reply to Ehsan Akhgari [:ehsan] from comment #19)
> > > Hrm, I thought that there was no work-around for the configure dependency?
> >
> > The workaround is a pile of #ifdef, and considering we do have such piles
> > for other stuff in MFBT, I'm inclined to say we should do the same.
>
> OK, I'm lost again. Which work-around are you talking about, and what are
> the examples of other places in MFBT where we do this?
Attributes.h, for example, has piles of ifdefs for some versions of compilers.
> Note that what we want to test here is whether the compiler can compile
> something like |void* foo = nullptr;| without including any headers, etc
> (which is what the configure check does.)
Is the information on http://wiki.apache.org/stdcxx/C++0xCompilerSupport enough?
Comment 24•12 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #23)
> (In reply to Ehsan Akhgari [:ehsan] from comment #22)
> > (In reply to Mike Hommey [:glandium] from comment #21)
> > > (In reply to Ehsan Akhgari [:ehsan] from comment #19)
> > > > Hrm, I thought that there was no work-around for the configure dependency?
> > >
> > > The workaround is a pile of #ifdef, and considering we do have such piles
> > > for other stuff in MFBT, I'm inclined to say we should do the same.
> >
> > OK, I'm lost again. Which work-around are you talking about, and what are
> > the examples of other places in MFBT where we do this?
>
> Attributes.h, for example, has piles of ifdefs for some versions of
> compilers.
I think that version detection on something like this is a bad idea in general. Doing this kind of stuff is likely to at least break all of our non Tier 1 platforms, and I don't particularly like my name to go on a patch which does that. But Jeff seems to be more open to this idea, so I'm assigning this bug to him.
> > Note that what we want to test here is whether the compiler can compile
> > something like |void* foo = nullptr;| without including any headers, etc
> > (which is what the configure check does.)
>
> Is the information on http://wiki.apache.org/stdcxx/C++0xCompilerSupport
> enough?
Not really, but it's a start. I think our position would be something like break all of the builds on platforms not on try, and get them to fix their builds. :(
Assignee: ehsan → jmuizelaar
Assignee | ||
Comment 25•12 years ago
|
||
Supports clang, msvc and gcc. This shouldn't break on any other compiler because using nullptr is opt-in.
Comment 26•12 years ago
|
||
Comment on attachment 657439 [details]
An implementation of NullPtr that does not rely on configure
Don't test clang versions when doing feature checks. The version numbers are purely marketing things, intended for vendor customization. They provide feature-testing macros which should be used instead:
http://clang.llvm.org/docs/LanguageExtensions.html#__has_feature_extension
http://clang.llvm.org/docs/LanguageExtensions.html#cxx_nullptr
For gcc and MSVC testing the version number is still the right way to go.
Comment 27•12 years ago
|
||
For gcc you need to check that C++11 is enabled ( __cplusplus >= 201203L || defined(__GXX_EXPERIMENTAL_CXX0X__). You probably need something similar with clang.
Comment 28•12 years ago
|
||
You should also take out the HAVE_NULLPTR check from configure.in.
Comment 29•12 years ago
|
||
And rename it MOZ_HAVE_CXX11_NULLPTR, for consitency with those in Attributes.h
Assignee | ||
Comment 30•12 years ago
|
||
I didn't use MOZ_HAVE_CXX11_NULLPTR because I didn't see the point in exposing that to users. Instead I undefine USE_NULLPTR after I'm done with it.
Attachment #657094 -
Attachment is obsolete: true
Attachment #657094 -
Flags: review?(mh+mozilla)
Attachment #657599 -
Flags: review?(mh+mozilla)
Comment 31•12 years ago
|
||
Comment on attachment 657599 [details] [diff] [review]
Patch (v3)
Review of attachment 657599 [details] [diff] [review]:
-----------------------------------------------------------------
Sending to waldo for the style nits (also because i'm not a peer)
::: mfbt/NullPtr.h
@@ +18,5 @@
> +# if __has_extension(cxx_nullptr)
> +# define USE_NULLPTR
> +# endif
> +#elif defined(__GNUC__)
> +# if defined(_GXX_EXPERIMENTAL_CXX0X__) || __cplusplus >= 201103L
missing an underscore to __GXX_EXPERIMENTAL_CXX0X__
@@ +19,5 @@
> +# define USE_NULLPTR
> +# endif
> +#elif defined(__GNUC__)
> +# if defined(_GXX_EXPERIMENTAL_CXX0X__) || __cplusplus >= 201103L
> +# if (__GNUC__*1000 + __GNU_MINOR__) >= 4006
I'd put spaces around *, but I don't know if it's the mfbt style.
@@ +24,5 @@
> +# define USE_NULLPTR
> +# endif
> +# endif
> +#elif _MSC_VER >= 1600
> +# define USE_NULLPTR
unaligned indentation
Attachment #657599 -
Flags: review?(mh+mozilla)
Attachment #657599 -
Flags: review?(jwalden+bmo)
Attachment #657599 -
Flags: review+
Comment 32•12 years ago
|
||
Comment on attachment 657599 [details] [diff] [review]
Patch (v3)
Review of attachment 657599 [details] [diff] [review]:
-----------------------------------------------------------------
::: mfbt/NullPtr.h
@@ +15,5 @@
> +# ifndef __has_extension
> +# define __has_extension __has_feature
> +# endif
> +# if __has_extension(cxx_nullptr)
> +# define USE_NULLPTR
The MOZ_* prefix isn't so much about exposing to users, as it is about making sure mfbt plays nice with any other code people might use it with. So this really should be MOZ_HAVE_CXX11_NULLPTR, then. Up to you if you want to #undef it; mostly we haven't #undef'd in the other cases because of laziness.
@@ +19,5 @@
> +# define USE_NULLPTR
> +# endif
> +#elif defined(__GNUC__)
> +# if defined(_GXX_EXPERIMENTAL_CXX0X__) || __cplusplus >= 201103L
> +# if (__GNUC__*1000 + __GNU_MINOR__) >= 4006
Yeah, spaces around binary operators.
Attachment #657599 -
Flags: review?(jwalden+bmo) → review+
Comment 33•12 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #32)
> The MOZ_* prefix isn't so much about exposing to users, as it is about
> making sure mfbt plays nice with any other code people might use it with.
> So this really should be MOZ_HAVE_CXX11_NULLPTR, then. Up to you if you
> want to #undef it; mostly we haven't #undef'd in the other cases because of
> laziness.
I'd leave it #def'd if there are any observable behavior differences that users might need to work around. I had to do this for enum class -- my drop-in replacement didn't work quite the same as the actual thing due to limitations of C++, so I had to check for that and fall back to different code to get nsresult-as-enum-class working.
Assignee | ||
Comment 34•12 years ago
|
||
Comment 35•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in
before you can comment on or make changes to this bug.
Description
•