Closed Bug 650304 Opened 14 years ago Closed 14 years ago

Use gcc C++0x mode by default, when it works as expected

Categories

(Firefox Build System :: General, defect)

All
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla6

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch (obsolete) (deleted) — Splinter Review
Now that we're getting near a switch to gcc 4.5, I think it's time to consider switching c++0x on by default with gcc, when it works and when it supports unicode literals (gcc 4.3, for instance has the c++0x experimental support but doesn't support unicode literals) FWIW, I've been building with -std=gnu++0x on Debian for a year, with gcc 4.4 and now 4.5, with no real issues but a few at the beginning (bug 502301) and one now (bug 640494). It has mainly two advantages: it avoids using a short wchar_t, which is always a good thing, and gcc c++0x support comes with extra checks that allowed to spot a real bug: https://bugzilla.mozilla.org/show_bug.cgi?id=502301#c10 bug 559278 is also related, but I don't think this bug needs to wait for it.
Attachment #526277 - Flags: review?(benjamin)
Attached patch Patch (deleted) — Splinter Review
Use GNU_CXX instead of GNU_CC
Attachment #526277 - Attachment is obsolete: true
Attachment #526277 - Flags: review?(benjamin)
Attachment #526464 - Flags: review?(benjamin)
Depends on: 652139
Attachment #526464 - Flags: review?(benjamin) → review+
Depends on: 654082
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
Depends on: 654493
Depends on: 654653
I think this bug should be backed out because of 2 reasons: 1. The patch lies about only making this change for gcc. It actually makes this change for any compiler which tries to be compatible with gcc, including clang, and perhaps icc too. 2. Our code base is not valid C++0x. Try compiling mozilla-central with the clang trunk, which implements the invalid conversion from unsigned types to their signed variants. This patch effectively causes our trunk to not be compilable with clang trunk, or any other gcc-masquerading C++0x conformant compiler. I think we should *only* take this patch if we have evidence that our code base compiles fine with a C++0x compiler (if there exists one yet).
(In reply to Ehsan Akhgari [:ehsan] from comment #3) > I think this bug should be backed out because of 2 reasons: > > 1. The patch lies about only making this change for gcc. It actually makes > this change for any compiler which tries to be compatible with gcc, > including clang, and perhaps icc too. It also only does so if the compiler supports the option. > 2. Our code base is not valid C++0x. Try compiling mozilla-central with the > clang trunk, which implements the invalid conversion from unsigned types to > their signed variants. > > This patch effectively causes our trunk to not be compilable with clang > trunk, or any other gcc-masquerading C++0x conformant compiler. I think we > should *only* take this patch if we have evidence that our code base > compiles fine with a C++0x compiler (if there exists one yet). It builds fine with gcc 4.5 and 4.6 with the option enabled. If there are problems that clang catches that gcc don't, we should fix these problems. BTW, I know that the js code doesn't compile with gcc with -std=gnu++0x (it fails with a problem like bug 502301). It so happens that the js code is not compiled with -std=gnu++0x.
Alternatively, we do have variables for icc and clang in configure, we could opt-out of -std=gnu++0x for them.
(In reply to Mike Hommey [:glandium] from comment #4) > It builds fine with gcc 4.5 and 4.6 with the option enabled. It does mean that we're effectively building with -std=gnu++0x on our linux buildbots.
I would ask for a day to track what is the problem clang is having. The reduced test is that in c++-0x one cannot drop precision in initialization without a cast, so things like int a = ...; char b = a; produce an error now that this part of c++0x has been implemented in clang. Fixing these is probably a good thing, but, as ehsan noted, we are sure to hit more of these as more of c++0x gets implemented.
I got firefox building with clang and c++0x again. I have started opening bugs with the patches. I don't think that checking clang out of the c++0x support is a good idea. Clang and gcc have implemented different subsets of c++0x so far. As the implementation progresses, it will find that some firefox code is invalid. I can see the value in moving us to c++0x as the compilers evolve or just reverting this patch until c++0x gets more mature, but it is hard to see the point in following the implementation of just one compiler.
OK, let's keep this in then. I still don't think that C++0x is mature enough for us to claim that our code is valid C++0x, but I don't want to argue too much over this.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: