Closed
Bug 865327
Opened 12 years ago
Closed 11 years ago
NullPtr.h:40:13: warning: 'nullptr' macro redefined (clang++ -stdlib=libc++)
Categories
(Core :: MFBT, defect)
Tracking
()
RESOLVED
INVALID
People
(Reporter: jbeich, Unassigned)
References
()
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
libc++ uses emulated nullptr_t when building without -std=c++11
which leads to a conflict with mfbt define for libstdc++.
dist/include/mozilla/NullPtr.h:40:13: warning: 'nullptr' macro redefined
# define nullptr __null
^
/usr/include/c++/v1/cstddef:87:9: note: previous definition is here
#define nullptr _VSTD::__get_nullptr_t()
^
1 warning generated.
There're a lot of such warnings under js/src.
Attachment #741398 -
Flags: review?(jmuizelaar)
Comment 2•12 years ago
|
||
Comment on attachment 741398 [details] [diff] [review]
prefer emulated
Review of attachment 741398 [details] [diff] [review]:
-----------------------------------------------------------------
I think this is ok. However, won't it break if you include NullPtr.h before a libc++ header?
Updated•12 years ago
|
Flags: needinfo?(jbeich)
Comment 3•12 years ago
|
||
We include a couple headers via the command line, before anything else. Perhaps this is another one we should add to that set.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> We include a couple headers via the command line, before anything else.
> Perhaps this is another one we should add to that set.
Then libc++ build (in non-c++11 mode) may miss emulated decltype in
parser/html/jArray.h due to MOZ_HAVE_CXX11_NULLPTR not being defined.
Shouldn't happen with v2.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #2)
> I think this is ok. However, won't it break if you include NullPtr.h before
> a libc++ header?
Not with clang parser.
$ cat -n test.cc
1 #ifdef mozilla_NullPtr_h_
2 #warning "mozilla/NullPtr.h" was included before
3 #endif
4
5 #include <cstddef>
6
7 #ifndef MOZ_HAVE_CXX11_NULLPTR
8 #error NullPtr.h used fallback
9 #endif
10
11 int main()
12 {
13 int foo = 5 | nullptr;
14 return 0;
15 }
# before (0th line or -include)
$ c++ -I dist/include -stdlib=libc++ test.cc
test.cc:4:2: warning: "mozilla/NullPtr.h" was included before
[-W#warnings]
#warning "mozilla/NullPtr.h" was included before
^
test.cc:10:2: error: NullPtr.h used fallback
#error NullPtr.h used fallback
^
test.cc:15:15: error: invalid operands to binary expression ('int' and
'std::__1::nullptr_t')
int foo = 5 | nullptr;
~ ^ ~~~~~~~
1 warning and 2 errors generated.
# after <cstddef> (6th line)
$ c++ -I dist/include -stdlib=libc++ test.cc
test.cc:14:15: error: invalid operands to binary expression ('int' and
'std::__1::nullptr_t')
int foo = 5 | nullptr;
~ ^ ~~~~~~~
1 error generated.
$ c++ -v
FreeBSD clang version 3.3 (trunk 178860) 20130405
Target: x86_64-unknown-freebsd10.0
Thread model: posix
# a bit debugging of "before" case
$ c++ -E -I dist/include -stdlib=libc++ test.cc 2>/dev/null |
sed -n '1,/cstddef/p'
# 1 "test.cc"
# 1 "<built-in>" 1
# 1 "<built-in>" 3
# 162 "<built-in>" 3
# 1 "<command line>" 1
# 1 "<built-in>" 2
# 1 "test.cc" 2
# 1 "dist/include/mozilla/NullPtr.h" 1
# 14 "dist/include/mozilla/NullPtr.h"
# 1 "dist/include/mozilla/Compiler.h" 1
# 15 "dist/include/mozilla/NullPtr.h" 2
# 2 "test.cc" 2
# 1 "/usr/include/c++/v1/cstddef" 1 3
Attachment #741398 -
Attachment is obsolete: true
Attachment #741398 -
Flags: review?(jmuizelaar)
Attachment #741579 -
Flags: review?(jwalden+bmo)
Attachment #741579 -
Flags: review?(jmuizelaar)
Flags: needinfo?(jbeich)
Comment 5•12 years ago
|
||
(In reply to comment #3)
> We include a couple headers via the command line, before anything else.
> Perhaps this is another one we should add to that set.
Yes, this is the correct fix here. I don't believe that we can protect against all possible edge cases by just hacking NullPtr.h itself.
Comment 6•12 years ago
|
||
Comment on attachment 741579 [details] [diff] [review]
prefer emulated, v2
I'm ok with this but I'll defer the real review to waldo.
Attachment #741579 -
Flags: review?(jmuizelaar)
Comment 7•12 years ago
|
||
Comment on attachment 741579 [details] [diff] [review]
prefer emulated, v2
Review of attachment 741579 [details] [diff] [review]:
-----------------------------------------------------------------
I think we probably do still just want to put NullPtr.h on the command line, but I also don't actually see anything this is going to get wrong. So let's run with this for now.
Attachment #741579 -
Flags: review?(jwalden+bmo) → review+
Comment 8•12 years ago
|
||
It's worth noting that when we have enough compiler support to assume nullptr, we want to be able to just remove NullPtr.h entirely. The #include <cstddef> may get in the way of that, by enabling people to bootleg its dependencies. But this was brought up when the Compiler.h include was added, and at the time it was decided just to deal with that problem when we have to, so I think we can do the same here.
Not an issue after bug 877937 and bug 895915.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → INVALID
You need to log in
before you can comment on or make changes to this bug.
Description
•