Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1083963002:
Remove duplicate -std=gnu++11 flag for Mac gn build (Closed)
|
Created:
5 years, 8 months ago by Jiang Jiang Modified:
5 years, 8 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove duplicate -std=gnu++11 flag for Mac gn build
It's already added when is_clang is true, which is always the case on
Mac.
BUG=336487
Committed: https://crrev.com/24bb916c8b5ed81187156c94f02cde89ea4da509
Cr-Commit-Position: refs/heads/master@{#325090}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Use gnu++11 for Linux and c++11 for rest of the non-win platforms #Patch Set 3 : Try using gnu++11 for android and nacl builds #Messages
Total messages: 27 (6 generated)
jiangj@opera.com changed reviewers: + brettw@chromium.org
thakis@chromium.org changed reviewers: + thakis@chromium.org
https://codereview.chromium.org/1083963002/diff/1/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/1083963002/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:415: cflags_cc += [ "-std=gnu++11" ] It should be -std=c++11 on non-linux
https://codereview.chromium.org/1083963002/diff/1/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/1083963002/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:415: cflags_cc += [ "-std=gnu++11" ] On 2015/04/14 15:17:16, Nico (away until Wed Apr 15) wrote: > It should be -std=c++11 on non-linux So basically: if (is_clang) { ... if (is_linux) { # gnu++11 instead of c++11 is needed because some code uses typeof() (a GNU # extension). # TODO(thakis): Eventually switch this to c++11 instead, # http://crbug.com/427584 cflags_cc += [ "-std=gnu++11" ] } else { cflags_cc += [ "-std=c++11" ] } } Sounds OK to you?
On 2015/04/14 15:28:44, Jiang Jiang wrote: > https://codereview.chromium.org/1083963002/diff/1/build/config/compiler/BUILD.gn > File build/config/compiler/BUILD.gn (right): > > https://codereview.chromium.org/1083963002/diff/1/build/config/compiler/BUILD... > build/config/compiler/BUILD.gn:415: cflags_cc += [ "-std=gnu++11" ] > On 2015/04/14 15:17:16, Nico (away until Wed Apr 15) wrote: > > It should be -std=c++11 on non-linux > > So basically: > > if (is_clang) { > ... > > if (is_linux) { > # gnu++11 instead of c++11 is needed because some code uses typeof() (a GNU > # extension). > # TODO(thakis): Eventually switch this to c++11 instead, > # http://crbug.com/427584 > cflags_cc += [ "-std=gnu++11" ] > } else { > cflags_cc += [ "-std=c++11" ] > } > } > > Sounds OK to you? I think the is_clang can be omitted. We always use c++11 (or gnu++11) these days.
Thanks for looking at this!
On 2015/04/14 15:45:21, Nico (away until Wed Apr 15) wrote: > I think the is_clang can be omitted. We always use c++11 (or gnu++11) these > days. Do we care about Windows non-clang builds here? I assume appending "-std=something" here will break MSVC builds.
On Tue, Apr 14, 2015 at 8:54 AM, <jiangj@opera.com> wrote: > On 2015/04/14 15:45:21, Nico (away until Wed Apr 15) wrote: > >> I think the is_clang can be omitted. We always use c++11 (or gnu++11) >> these >> days. >> > > Do we care about Windows non-clang builds here? I assume appending > "-std=something" here will break MSVC builds. > Yes, we shouldn't do this if is_win (independent of clang; clang uses a cl.exe-compatible driver on Windows and not the gcc flags. But clang/win doesn't work in the gn build yet anyways.) To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/04/14 15:57:41, Nico (away until Wed Apr 15) wrote: > On Tue, Apr 14, 2015 at 8:54 AM, <mailto:jiangj@opera.com> wrote: > > > On 2015/04/14 15:45:21, Nico (away until Wed Apr 15) wrote: > > > >> I think the is_clang can be omitted. We always use c++11 (or gnu++11) > >> these > >> days. > >> > > > > Do we care about Windows non-clang builds here? I assume appending > > "-std=something" here will break MSVC builds. > > > > Yes, we shouldn't do this if is_win (independent of clang; clang uses a > cl.exe-compatible driver on Windows and not the gcc flags. But clang/win > doesn't work in the gn build yet anyways.) > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Updated. I guess we no longer care about gcc_version < 48 builds.
lgtm if the trybots like it (I'm not sure what gyp does for android)
The CQ bit was checked by jiangj@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1083963002/20001
The CQ bit was unchecked by jiangj@opera.com
lgtm
On 2015/04/14 16:06:24, Nico (away until Wed Apr 15) wrote: > lgtm if the trybots like it (I'm not sure what gyp does for android) Looks like Android builds doesn't use c++11 yet. http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...
On 2015/04/14 16:12:27, brettw wrote: > lgtm Any idea why Windows gn trybot was failing? http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gn_...
Also it appears that nacl builds requires a different flag: http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... I never worked with nacl so have no clue here, am I supposed to use -std=gnu++11 for is_nacl?
Also Mac gn trybot failure (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_d...) appears to be a separate issue, I'm trying to fix it at https://codereview.chromium.org/1052403003/.
On 2015/04/14 16:23:40, Jiang Jiang wrote: > Also it appears that nacl builds requires a different flag: > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... > > I never worked with nacl so have no clue here, am I supposed to use -std=gnu++11 > for is_nacl? I believe nacl uses -std=gnu++11 unconditionally (gnu++0x even, since some of their compilers are – or used to be – really old): https://code.google.com/p/chromium/codesearch#chromium/src/native_client/buil... Does nacl not use build_nexe.py with gn? I suppose one of the reasons for gn is to get rid of build_nexe.py, so that makes sense. So yeah, nacl needs a special case too.
On 2015/04/14 16:19:21, Jiang Jiang wrote: > On 2015/04/14 16:12:27, brettw wrote: > > lgtm > > Any idea why Windows gn trybot was failing? > > http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gn_... No, but seems to happen on more than 50% of the runs on that bot: http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gn_... :-( File a bug, cc dpranke I guess :-/
On 2015/04/14 16:41:41, Nico (away until Wed Apr 15) wrote: > On 2015/04/14 16:19:21, Jiang Jiang wrote: > > On 2015/04/14 16:12:27, brettw wrote: > > > lgtm > > > > Any idea why Windows gn trybot was failing? > > > > > http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gn_... > > No, but seems to happen on more than 50% of the runs on that bot: > http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gn_... > :-( > > File a bug, cc dpranke I guess :-/ Done: http://crbug.com/476977
The CQ bit was checked by jiangj@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/1083963002/#ps40001 (title: "Try using gnu++11 for android and nacl builds")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1083963002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/24bb916c8b5ed81187156c94f02cde89ea4da509 Cr-Commit-Position: refs/heads/master@{#325090} |
