#define nullptr __null for gcc <= 4.5

RESOLVED FIXED in Firefox 17

Status

()

Core
MFBT
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

Trunk
mozilla17
Points:
---

Firefox Tracking Flags

(firefox17 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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
I think we need another configure check for this, which kicks in when nullptr is not available.
(Assignee)

Comment 2

5 years ago
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.
(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!
(Assignee)

Comment 4

5 years ago
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
Attachment #646898 - Flags: feedback?(ehsan)
We use gcc 4.5 on the linux build slaves.
Comment on attachment 646898 [details] [diff] [review]
WIP-define-nullptr-__null.patch

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

Looks good, ship it!
Attachment #646898 - Flags: feedback?(ehsan) → review+
(Assignee)

Comment 7

5 years ago
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!
Assignee: nobody → cpeterson
Status: NEW → ASSIGNED
(Assignee)

Comment 8

5 years ago
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.
(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!
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae45a2c9d678
status-firefox17: --- → fixed
(Assignee)

Comment 11

5 years ago
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).
https://hg.mozilla.org/mozilla-central/rev/ae45a2c9d678
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Flagging [qa-] as I don't think this needs QA verification. Please correct me if I am wrong.
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.