Last Comment Bug 777511 - #define nullptr __null for gcc <= 4.5
: #define nullptr __null for gcc <= 4.5
Status: RESOLVED FIXED
[qa-]
:
Product: Core
Classification: Components
Component: MFBT (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Chris Peterson [:cpeterson]
:
Mentors:
Depends on:
Blocks: 626472
  Show dependency treegraph
 
Reported: 2012-07-25 14:34 PDT by Chris Peterson [:cpeterson]
Modified: 2012-10-17 16:47 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
WIP-define-nullptr-__null.patch (1.10 KB, patch)
2012-07-28 12:45 PDT, Chris Peterson [:cpeterson]
ehsan: review+
Details | Diff | Splinter Review

Description Chris Peterson [:cpeterson] 2012-07-25 14:34:29 PDT
gcc did not add nullptr until gcc 4.6 [1]. For versions of gcc that do not HAVE_NULLPTR (i.e. gcc <= 4.5 and Android's gcc 4.4), we should consider mapping nullptr to gcc's magic __null.

Something like:

#ifndef HAVE_NULLPTR
#if defined(_WIN64)
# define nullptr 0LL
#elif defined(__GNUC__)
# define nullptr __null
#else
# define nullptr 0L
#endif
#endif /* defined(HAVE_NULLPTR) */


[1] http://gcc.gnu.org/projects/cxx0x.html
Comment 1 :Ehsan Akhgari 2012-07-25 14:48:54 PDT
I think we need another configure check for this, which kicks in when nullptr is not available.
Comment 2 Chris Peterson [:cpeterson] 2012-07-25 15:49:18 PDT
I believe g++ has had __null since at least g++ 2.95, so a configure check might be overkill. :)

Note that __null is only available with g++, not gcc. g++ implicitly #defines NULL __null for all C++ code.
Comment 3 :Ehsan Akhgari 2012-07-26 15:05:57 PDT
(In reply to comment #2)
> I believe g++ has had __null since at least g++ 2.95, so a configure check
> might be overkill. :)

Hehe, ok.

> Note that __null is only available with g++, not gcc. g++ implicitly #defines
> NULL __null for all C++ code.

Good point!
Comment 4 Chris Peterson [:cpeterson] 2012-07-28 12:45:27 PDT
Created attachment 646898 [details] [diff] [review]
WIP-define-nullptr-__null.patch

Ehsan, this patch is a WIP for your feedback.

This patch proposes two changes:

 1. For C code, #define nullptr (and nsnull) as a traditional C-style void pointer. This will give us warnings if C code tries to assign nullptr to an int. gcc's magic __null is only available in C++ code.

 2. For C++ code compiled with gcc versions that don't HAVE_NULLPTR (i.e. gcc <= 4.5), #define nullptr (and nsnull) as gcc's magic __null. 

Unfortunately, __null is mostly useless in gcc <= 4.4 because those versions treat __null exactly like 0L unless you enable -Wconversion warnings. And we don't what to enable that -Wconversion because it is notorious for giving tons of useless warnings and false positives. gcc 4.5 fixed that problem by adding -Wconversion-null (enabled by default) to emit just the __null conversion warnings.

gcc 4.6 introduced nullptr and will HAVE_NULLPTR, so this __null patch would *only* benefit gcc 4.5. Do we even use gcc 4.5 on any platforms? Which gcc version do we use on Linux? Android is stuck on gcc 4.4 and Mac OS X dumped gcc for clang.


A tryserver build of mozilla-central + my patch was mostly green (except Android XUL):
https://tbpl.mozilla.org/?tree=Try&rev=4b17728ebe07
Comment 5 Mike Hommey [:glandium] 2012-07-29 01:00:59 PDT
We use gcc 4.5 on the linux build slaves.
Comment 6 :Ehsan Akhgari 2012-07-30 09:38:50 PDT
Comment on attachment 646898 [details] [diff] [review]
WIP-define-nullptr-__null.patch

Review of attachment 646898 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, ship it!
Comment 7 Chris Peterson [:cpeterson] 2012-07-30 15:04:32 PDT
Given my patch and the following test code:

  char a = nullptr;
  int  b = nullptr;
  char c = NULL;
  int  d = NULL;

* gcc 4.4 emits NO warnings for Android or B2G, as expected. :(

* gcc 4.5 emits these warnings on Linux:

  warning: converting to non-pointer type 'char' from NULL
  warning: converting to non-pointer type 'char' from NULL
  warning: converting to non-pointer type 'int' from NULL
  warning: converting to non-pointer type 'int' from NULL

* clangs emits these NULL warnings and nullptr errors on OS X:

  error: assigning to 'char' from incompatible type 'nullptr_t'
  error: assigning to 'int' from incompatible type 'nullptr_t'
  warning: implicit conversion of NULL constant to 'char' [-Wnull-conversion]

Note that clang warns about implicit conversion of nullptr to int, but not NULL to int!
Comment 8 Chris Peterson [:cpeterson] 2012-07-30 15:05:43 PDT
1. To make gcc's warnings fatal like clang's nullptr errors, we may want to add -Werror=conversion-null to Linux's CFLAGS/CXXFLAGS for gcc >= 4.5. (The -Wconversion-null flag was new in gcc 4.5.)

If a developer overlooks these gcc warnings, then they may not realize their code change will break the OS X build until they push to the tryserver. This a waste of developer time and tryserver resources.

2. To make clang's NULL warnings fatal like its nullptr errors, we may want to add -Werror=null-conversion to clang's CFLAGS/CXXFLAGS.
Comment 9 :Ehsan Akhgari 2012-07-30 15:09:25 PDT
(In reply to comment #8)
> 1. To make gcc's warnings fatal like clang's nullptr errors, we may want to add
> -Werror=conversion-null to Linux's CFLAGS/CXXFLAGS for gcc >= 4.5. (The
> -Wconversion-null flag was new in gcc 4.5.)
> 
> If a developer overlooks these gcc warnings, then they may not realize their
> code change will break the OS X build until they push to the tryserver. This a
> waste of developer time and tryserver resources.
> 
> 2. To make clang's NULL warnings fatal like its nullptr errors, we may want to
> add -Werror=null-conversion to clang's CFLAGS/CXXFLAGS.

I think these are both good ideas.  Can you please get bugs filed for them?  Thanks!
Comment 10 Chris Peterson [:cpeterson] 2012-07-30 15:29:11 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae45a2c9d678
Comment 11 Chris Peterson [:cpeterson] 2012-07-30 16:21:42 PDT
bug 778980
(In reply to Ehsan Akhgari [:ehsan] from comment #9)
> I think these are both good ideas.  Can you please get bugs filed for them? 

I opened bug 778980 and bug 778984 (and linked them to master bug 626472).
Comment 12 Ed Morley [:emorley] 2012-07-31 06:11:50 PDT
https://hg.mozilla.org/mozilla-central/rev/ae45a2c9d678
Comment 13 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-10-17 16:47:49 PDT
Flagging [qa-] as I don't think this needs QA verification. Please correct me if I am wrong.

Note You need to log in before you can comment on or make changes to this bug.