Closed Bug 782647 Opened 12 years ago Closed 12 years ago

Move nullptr to MFBT

Categories

(Core :: MFBT, defect)

defect
Not set
normal

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
Assignee: nobody → Ms2ger
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #652179 - Flags: review?(jones.chris.g)
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 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.
I'll just drop that; it's clear enough from the code.
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)
(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 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-
As said on IRC today, I think we should take out the C fallback case.
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.
(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?
(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?
Assignee: Ms2ger → nobody
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. :/
(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.
(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.  :/ )
That sounds cool.
Attached patch Patch (v2) (obsolete) — Splinter Review
Assignee: nobody → ehsan
Attachment #652179 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #657094 - Flags: review?(jwalden+bmo)
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+
Hrm, I thought that there was no work-around for the configure dependency?
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
(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.
(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.)
(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?
(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
Supports clang, msvc and gcc. This shouldn't break on any other compiler because using nullptr is opt-in.
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.
For gcc you need to check that C++11 is enabled ( __cplusplus >= 201203L || defined(__GXX_EXPERIMENTAL_CXX0X__). You probably need something similar with clang.
You should also take out the HAVE_NULLPTR check from configure.in.
And rename it MOZ_HAVE_CXX11_NULLPTR, for consitency with those in Attributes.h
Attached patch Patch (v3)Splinter Review
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 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 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+
(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.
https://hg.mozilla.org/mozilla-central/rev/79a25e159bfd
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Depends on: 818941
Depends on: 865327
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: