Closed
Bug 689609
Opened 13 years ago
Closed 12 years ago
We enable c++0x in places it is not supported
Categories
(Firefox Build System :: General, defect)
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: espindola, Assigned: espindola)
References
Details
Attachments
(1 file, 1 obsolete file)
2.28 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Attachment #562788 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 1•13 years ago
|
||
The problem that I found is that android uses gcc 4.4 which has the option, but uses a libstdc++ with no c++0x support.
Comment 2•13 years ago
|
||
Comment on attachment 562788 [details] [diff] [review] Use a more strict c++0x test Review of attachment 562788 [details] [diff] [review]: ----------------------------------------------------------------- ::: configure.in @@ +2832,5 @@ > _SAVE_CXXFLAGS=$CXXFLAGS > CXXFLAGS="$CXXFLAGS -std=gnu++0x" > > + CXXFLAGS="$CXXFLAGS $_MOZ_RTTI_FLAGS" > + AC_CACHE_CHECK(for c++0x support, Please make it clear that this is only testing the subset that we know we need for our codebase.
Attachment #562788 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 3•13 years ago
|
||
Try at https://tbpl.mozilla.org/?tree=Try&rev=6c4ab8e9314f
Assignee | ||
Comment 4•13 years ago
|
||
The failure on Android is because ANPCanvas.cpp has a dependency on c++11!!! The reduced case is enum foo{ bar = 1 }; int zed = foo::bar; which is valid c++11, but not valid c++03.
Comment 5•13 years ago
|
||
OK, I've objected to building our code in C++11 mode before, but let me rephrase my objection again (and make it stronger): * Comment 4 includes an example of a valid C++11 program which is not valid C++98. * Building our code in C++11 mode has caused this pattern to crawl its way into our codebase. As a result, our Firefox Android code is no longer valid C++98. * C++11 doesn't currently have any complete implementations, so there is no way for us to know whether our code is valid C++11 by compiling it under gcc/clang c++0x mode. Therefore, I think we should take out that mode as the default build option for Firefox on all platforms. I certainly will not object to people building m-c clones under c++0x mode in gcc/clang locally, and submit patches for build failures. I won't even object to us having Tier-2/3 builders which compile Firefox under c++0x mode. What I do object to is having that flag enabled by default which has clearly lead to us accept non-valid C++98 code. glandium, do you agree?
Assignee | ||
Updated•13 years ago
|
Attachment #562788 -
Attachment is obsolete: true
Assignee | ||
Comment 6•13 years ago
|
||
This issue changed my opinion. I liked having the build use c++11 when possible, since it added some interesting checks (like the initializer lists) and got us ready for newer compilers. What I didn't realize is that it made so easy to create a dependency on an experimental feature. Is it easy to set up a bot that does a daily build of m-c with c++0x? I would be more than happy to keep an eye on it like I do with clang builds.
Comment 7•13 years ago
|
||
I don't disagree, but: (In reply to Ehsan Akhgari [:ehsan] from comment #5) > * Building our code in C++11 mode has caused this pattern to crawl its way > into our codebase. As a result, our Firefox Android code is no longer valid > C++98. Should we actually care? I mean, there's not a lot of different NDKs, and at least all those >= r4 are using the same compiler version. So, as long as everyone is using the same mode, there's no problem. > * C++11 doesn't currently have any complete implementations, so there is no > way for us to know whether our code is valid C++11 by compiling it under > gcc/clang c++0x mode. On the other hand, on Linux and Android, not using c++0x mode means relying on short wchar_t, which has possible much worse issues. If only there was a way to have char16_t and utf-16 string literals without c++0x...
Assignee | ||
Comment 8•13 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #7) > I don't disagree, but: > > (In reply to Ehsan Akhgari [:ehsan] from comment #5) > > * Building our code in C++11 mode has caused this pattern to crawl its way > > into our codebase. As a result, our Firefox Android code is no longer valid > > C++98. > > Should we actually care? I mean, there's not a lot of different NDKs, and at > least all those >= r4 are using the same compiler version. So, as long as > everyone is using the same mode, there's no problem. It will also be an issue with OS X only code when we switch to clang. In the other hand, gcc 4.2 is so old that we would probably get incompatible with it fairly quickly anyway.
Assignee | ||
Comment 9•13 years ago
|
||
New try: https://tbpl.mozilla.org/?tree=Try&rev=2551eae56a6c
Comment 10•13 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #7) > (In reply to Ehsan Akhgari [:ehsan] from comment #5) > > * Building our code in C++11 mode has caused this pattern to crawl its way > > into our codebase. As a result, our Firefox Android code is no longer valid > > C++98. > > Should we actually care? I mean, there's not a lot of different NDKs, and at > least all those >= r4 are using the same compiler version. So, as long as > everyone is using the same mode, there's no problem. Yes, we should care. Note that this is not an Android only issue. This is going to continuously bite us as the compilers change, and it could potentially bite us even more if we get some code in our cross platform code which results in different side effects because of different compilers implementing C++11 differently. C++11 support at this point is a highly moving target in the compilers. I'm all for us being future ready, but the fact is that we can't use C++11 features in our code yet, so turning it on by default seems like the wrong choice to me. > > * C++11 doesn't currently have any complete implementations, so there is no > > way for us to know whether our code is valid C++11 by compiling it under > > gcc/clang c++0x mode. > > On the other hand, on Linux and Android, not using c++0x mode means relying > on short wchar_t, which has possible much worse issues. > > If only there was a way to have char16_t and utf-16 string literals without > c++0x... Which issues are you talking about? Calling into system libraries implementing things like wcslen? Can we just handle these problems the same way as we did before enabling c++0x mode?
Comment 11•13 years ago
|
||
(In reply to Rafael Ávila de Espíndola (:espindola) from comment #6) > Is it easy to set up a bot that does a daily build of m-c with c++0x? I > would be more than happy to keep an eye on it like I do with clang builds. It's definitely possible, it's something that RelEng needs to do so it might be a bit before they get the builds up and running. But in the mean time a bunch of us can build in C++0x mode locally on a regular basis. I'd be happy to do that myself, and I'm sure that we can find a few other people as well. :-)
Comment 12•13 years ago
|
||
(In reply to Ehsan Akhgari [:ehsan] from comment #10) > Which issues are you talking about? Calling into system libraries > implementing things like wcslen? Can we just handle these problems the same > way as we did before enabling c++0x mode? That's exactly where the problem is: before enabling c++0x mode, we were relying on -fshort-wchar, and the more we use the STL, the more it is dangerous.
Assignee | ||
Comment 13•13 years ago
|
||
This is the same patch you approved last time, but fixes the use of c++11 features in the android code. Since this disables c++11 for android (when not using libstdc++), it is also on the right path if we decide to disable it completely.
Assignee | ||
Comment 14•13 years ago
|
||
> That's exactly where the problem is: before enabling c++0x mode, we were
> relying on -fshort-wchar, and the more we use the STL, the more it is
> dangerous.
Can you use the system you wrote for checking for unwanted dependencies on newer versions of libstdc++? We could check that no wchar_t is present in a name? For example, in
#include <vector>
int foobar(wchar_t x, std::vector<wchar_t> &y) {
y.push_back(x);
}
I get a weak copy of _ZNSt6vectorIwSaIwEE9push_backERKw, but in a different libstdc++ it could be a U, which would be bad, but can be found by looking at the libstdc++ symbols that XUL uses.
Comment 15•13 years ago
|
||
(In reply to Rafael Ávila de Espíndola (:espindola) from comment #14) > Can you use the system you wrote for checking for unwanted dependencies on > newer versions of libstdc++? Nope, it doesn't do the same thing at all.
Assignee | ||
Comment 16•13 years ago
|
||
In any case, we cannot really use char16_t is core firefox until we switch to clang and vc 2010, right? Until then, the value of having c++0x by default in compilers that support it is much reduced.
Comment 17•13 years ago
|
||
(In reply to Rafael Ávila de Espíndola (:espindola) from comment #16) > In any case, we cannot really use char16_t is core firefox until we switch > to clang and vc 2010, right? We effectively use it with gcc now, but that's limited to NS_LITERAL_STRINGs.
Assignee | ||
Comment 18•13 years ago
|
||
OK. We should probably move the more general c++0x discussion to another bug or list and keep this bug for just the smaller issue of not enabling it when it is not supported.
Comment 19•13 years ago
|
||
Comment on attachment 563126 [details] [diff] [review] better check for c++11 r=me on the ANPCanvas.cpp part. We definitely want this patch for Android. I'll post something to dev.platform on the general question of whether or not we should use c++0x mode.
Attachment #563126 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 20•13 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fce3a720570
Comment 21•13 years ago
|
||
Parts of this patch were backed out and/or re-landed before being merged. I'm lost track and am not sure whether to resolve it or not. http://hg.mozilla.org/mozilla-central/rev/4fce3a720570 http://hg.mozilla.org/mozilla-central/rev/0c9d0f4f1c82 http://hg.mozilla.org/mozilla-central/rev/51cc78c7433d
Assignee | ||
Comment 22•12 years ago
|
||
We are stuck with c++11 on android. Things will get better when we upgrade the compiler we use there, but we are not dropping c++11. Closing this as wontfix.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•