Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2093543003:
[Mac/iOS/GN] Remove bad -Wno-deprecated-declarations from //third_party/google_toolbox_for_mac publ… (Closed)
|
Created:
4 years, 6 months ago by Robert Sesek Modified:
4 years, 6 months ago Reviewers:
Nico CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@gn-missing-objc-flags Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Mac/iOS/GN] Remove bad -Wno-deprecated-declarations from //third_party/google_toolbox_for_mac public_config.
BUG=622889, 622481
R=thakis@chromium.org
Committed: https://crrev.com/5d8c668952c6972eb1f833fddccef7d2fd66048b
Cr-Commit-Position: refs/heads/master@{#401809}
Patch Set 1 #Patch Set 2 #Patch Set 3 : remove unneeded config #
Total comments: 2
Messages
Total messages: 13 (2 generated)
lgtm
The CQ bit was checked by rsesek@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2093543003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Mac/iOS/GN] Remove bad -Wno-deprecated-declarations from //third_party/google_toolbox_for_mac public_config. BUG=622889,622481 R=thakis@chromium.org ========== to ========== [Mac/iOS/GN] Remove bad -Wno-deprecated-declarations from //third_party/google_toolbox_for_mac public_config. BUG=622889,622481 R=thakis@chromium.org Committed: https://crrev.com/5d8c668952c6972eb1f833fddccef7d2fd66048b Cr-Commit-Position: refs/heads/master@{#401809} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5d8c668952c6972eb1f833fddccef7d2fd66048b Cr-Commit-Position: refs/heads/master@{#401809}
Message was sent while issue was closed.
https://codereview.chromium.org/2093543003/diff/40001/third_party/google_tool... File third_party/google_toolbox_for_mac/BUILD.gn (right): https://codereview.chromium.org/2093543003/diff/40001/third_party/google_tool... third_party/google_toolbox_for_mac/BUILD.gn:191: cflags = [ "-Wno-deprecated-declarations" ] oh, fyi, in general putting -Wno-foo flags in cflags instead of a config doesn't work. Since this target removes chromium_code and adds no_chromium_code, no_chromium_code is pretty late in the config list, and its flags get added before the cflags here. no_chromium_code does contain -Wall, so -Wall will appear after -Wno-foo, and if foo is in -Wall then the -Wall will reenable the warning that -Wno-foo disabled. In this case, Wdeprecated-declarations apparently isn't in -Wall, which might be why this works. (Or the flag isn't needed anymore?)
Message was sent while issue was closed.
https://codereview.chromium.org/2093543003/diff/40001/third_party/google_tool... File third_party/google_toolbox_for_mac/BUILD.gn (right): https://codereview.chromium.org/2093543003/diff/40001/third_party/google_tool... third_party/google_toolbox_for_mac/BUILD.gn:191: cflags = [ "-Wno-deprecated-declarations" ] On 2016/06/24 19:17:40, Nico wrote: > oh, fyi, in general putting -Wno-foo flags in cflags instead of a config doesn't > work. Since this target removes chromium_code and adds no_chromium_code, > no_chromium_code is pretty late in the config list, and its flags get added > before the cflags here. no_chromium_code does contain -Wall, so -Wall will > appear after -Wno-foo, and if foo is in -Wall then the -Wall will reenable the > warning that -Wno-foo disabled. In this case, Wdeprecated-declarations > apparently isn't in -Wall, which might be why this works. (Or the flag isn't > needed anymore?) Hm. It is necessary on iOS and not on Mac (see ps1 failures). Should I switch these to a config for iOS? It currently is working as intended.
Message was sent while issue was closed.
On 2016/06/24 19:59:25, Robert Sesek wrote: > https://codereview.chromium.org/2093543003/diff/40001/third_party/google_tool... > File third_party/google_toolbox_for_mac/BUILD.gn (right): > > https://codereview.chromium.org/2093543003/diff/40001/third_party/google_tool... > third_party/google_toolbox_for_mac/BUILD.gn:191: cflags = [ > "-Wno-deprecated-declarations" ] > On 2016/06/24 19:17:40, Nico wrote: > > oh, fyi, in general putting -Wno-foo flags in cflags instead of a config > doesn't > > work. Since this target removes chromium_code and adds no_chromium_code, > > no_chromium_code is pretty late in the config list, and its flags get added > > before the cflags here. no_chromium_code does contain -Wall, so -Wall will > > appear after -Wno-foo, and if foo is in -Wall then the -Wall will reenable the > > warning that -Wno-foo disabled. In this case, Wdeprecated-declarations > > apparently isn't in -Wall, which might be why this works. (Or the flag isn't > > needed anymore?) > > Hm. It is necessary on iOS and not on Mac (see ps1 failures). Should I switch > these to a config for iOS? It currently is working as intended. Since this suppression will hopefully go away soon, it's probably fine to keep as-is. Just wanted to make you aware of this subtlety.
Message was sent while issue was closed.
On 2016/06/24 21:37:21, Nico wrote: > On 2016/06/24 19:59:25, Robert Sesek wrote: > > > https://codereview.chromium.org/2093543003/diff/40001/third_party/google_tool... > > File third_party/google_toolbox_for_mac/BUILD.gn (right): > > > > > https://codereview.chromium.org/2093543003/diff/40001/third_party/google_tool... > > third_party/google_toolbox_for_mac/BUILD.gn:191: cflags = [ > > "-Wno-deprecated-declarations" ] > > On 2016/06/24 19:17:40, Nico wrote: > > > oh, fyi, in general putting -Wno-foo flags in cflags instead of a config > > doesn't > > > work. Since this target removes chromium_code and adds no_chromium_code, > > > no_chromium_code is pretty late in the config list, and its flags get added > > > before the cflags here. no_chromium_code does contain -Wall, so -Wall will > > > appear after -Wno-foo, and if foo is in -Wall then the -Wall will reenable > the > > > warning that -Wno-foo disabled. In this case, Wdeprecated-declarations > > > apparently isn't in -Wall, which might be why this works. (Or the flag isn't > > > needed anymore?) > > > > Hm. It is necessary on iOS and not on Mac (see ps1 failures). Should I switch > > these to a config for iOS? It currently is working as intended. > > Since this suppression will hopefully go away soon, it's probably fine to keep > as-is. Just wanted to make you aware of this subtlety. Yup, thanks. Luckily that's documented (`gn help configs`).
Message was sent while issue was closed.
config order is documented, but it still took me a bit to figure out that -Wall can override an earlier -Wno-foo. If it's clear to you, more power to you thou :-) On Fri, Jun 24, 2016 at 5:52 PM, <rsesek@chromium.org> wrote: > On 2016/06/24 21:37:21, Nico wrote: > > On 2016/06/24 19:59:25, Robert Sesek wrote: > > > > > > > https://codereview.chromium.org/2093543003/diff/40001/third_party/google_tool... > > > File third_party/google_toolbox_for_mac/BUILD.gn (right): > > > > > > > > > > https://codereview.chromium.org/2093543003/diff/40001/third_party/google_tool... > > > third_party/google_toolbox_for_mac/BUILD.gn:191: cflags = [ > > > "-Wno-deprecated-declarations" ] > > > On 2016/06/24 19:17:40, Nico wrote: > > > > oh, fyi, in general putting -Wno-foo flags in cflags instead of a > config > > > doesn't > > > > work. Since this target removes chromium_code and adds > no_chromium_code, > > > > no_chromium_code is pretty late in the config list, and its flags get > added > > > > before the cflags here. no_chromium_code does contain -Wall, so > -Wall will > > > > appear after -Wno-foo, and if foo is in -Wall then the -Wall will > reenable > > the > > > > warning that -Wno-foo disabled. In this case, > Wdeprecated-declarations > > > > apparently isn't in -Wall, which might be why this works. (Or the > flag > isn't > > > > needed anymore?) > > > > > > Hm. It is necessary on iOS and not on Mac (see ps1 failures). Should I > switch > > > these to a config for iOS? It currently is working as intended. > > > > Since this suppression will hopefully go away soon, it's probably fine > to keep > > as-is. Just wanted to make you aware of this subtlety. > > Yup, thanks. Luckily that's documented (`gn help configs`). > > https://codereview.chromium.org/2093543003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2016/06/24 21:54:30, Nico wrote: > config order is documented, but it still took me a bit to figure out that > -Wall can override an earlier -Wno-foo. If it's clear to you, more power to > you thou :-) Maybe someone made it more clear recently? :) """ Configs on a target When used on a target, the include_dirs, defines, etc. in each config are appended in the order they appear to the compile command for each file in the target. They will appear after the include_dirs, defines, etc. that the target sets directly. """ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
