The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla6

Status

()

Core
Build Config
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

Trunk
mozilla6
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Created attachment 526277 [details] [diff] [review]
Patch

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)
(Assignee)

Comment 1

6 years ago
Created attachment 526464 [details] [diff] [review]
Patch

Use GNU_CXX instead of GNU_CC
Attachment #526277 - Attachment is obsolete: true
Attachment #526277 - Flags: review?(benjamin)
Attachment #526464 - Flags: review?(benjamin)
(Assignee)

Updated

6 years ago
Depends on: 652139
Attachment #526464 - Flags: review?(benjamin) → review+
(Assignee)

Updated

6 years ago
Depends on: 654082
(Assignee)

Comment 2

6 years ago
http://hg.mozilla.org/mozilla-central/rev/7528b2718827
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
(Assignee)

Updated

6 years ago
Depends on: 654493

Updated

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

Comment 4

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

Comment 5

6 years ago
Alternatively, we do have variables for icc and clang in configure, we could opt-out of -std=gnu++0x for them.
(Assignee)

Comment 6

6 years ago
(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.
You need to log in before you can comment on or make changes to this bug.