777511 - #define nullptr __null for gcc <= 4.5

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Chris Peterson [:cpeterson]]

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

[(no longer active)]

I think we need another configure check for this, which kicks in when nullptr is not available.

Comment 2

[Chris Peterson [:cpeterson]]

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

[(no longer active)]

(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]]

Attachment: 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]]

We use gcc 4.5 on the linux build slaves.

Comment 6

[(no longer active)]

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]]

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]]

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

[(no longer active)]

(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]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae45a2c9d678

Comment 11

[Chris Peterson [:cpeterson]]

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]]

https://hg.mozilla.org/mozilla-central/rev/ae45a2c9d678

Comment 13

[u279076]

Flagging [qa-] as I don't think this needs QA verification. Please correct me if I am wrong.