Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2404283002:
[gn] Add back two warnings V8 uses with gyp (Closed)
|
Created:
4 years, 2 months ago by Michael Achenbach Modified:
4 years, 2 months ago CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[gn] Add back two warnings V8 uses with gyp
BUG=428099
Committed: https://crrev.com/f18a9ad780a71121c2e2cb98d7a869e5b5b38478
Cr-Commit-Position: refs/heads/master@{#40235}
Patch Set 1 #Patch Set 2 : [gn] Add back two warnings V8 uses with gyp #
Total comments: 4
Messages
Total messages: 24 (17 generated)
The CQ bit was checked by machenbach@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
The CQ bit was checked by machenbach@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/14317) v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/14332)
The CQ bit was checked by machenbach@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [gn] Add back two warnings V8 uses with gyp BUG= ========== to ========== [gn] Add back two warnings V8 uses with gyp BUG=428099 ==========
machenbach@chromium.org changed reviewers: + clemensh@chromium.org, hans@chromium.org, thakis@chromium.org
PTAL https://codereview.chromium.org/2404283002/diff/20001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/2404283002/diff/20001/BUILD.gn#newcode378 BUILD.gn:378: "-Winconsistent-missing-override", I hope this actually switches it on. The gn defaults add -Wno-inconsistent-missing-override and we append this afterwards.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm. Thanks Michael! https://codereview.chromium.org/2404283002/diff/20001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/2404283002/diff/20001/BUILD.gn#newcode378 BUILD.gn:378: "-Winconsistent-missing-override", On 2016/10/11 12:55:35, machenbach (slow) wrote: > I hope this actually switches it on. The gn defaults add > -Wno-inconsistent-missing-override and we append this afterwards. A local test confirms that it should be switched on by this change.
https://codereview.chromium.org/2404283002/diff/20001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/2404283002/diff/20001/BUILD.gn#newcode378 BUILD.gn:378: "-Winconsistent-missing-override", On 2016/10/11 13:00:26, Clemens Hammacher wrote: > On 2016/10/11 12:55:35, machenbach (slow) wrote: > > I hope this actually switches it on. The gn defaults add > > -Wno-inconsistent-missing-override and we append this afterwards. > > A local test confirms that it should be switched on by this change. This works by luck I think. The usual way is to put warning flags in a config (https://cs.chromium.org/chromium/src/third_party/mesa/BUILD.gn?rcl=0&l=119)
https://codereview.chromium.org/2404283002/diff/20001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/2404283002/diff/20001/BUILD.gn#newcode378 BUILD.gn:378: "-Winconsistent-missing-override", On 2016/10/11 13:39:37, Nico wrote: > On 2016/10/11 13:00:26, Clemens Hammacher wrote: > > On 2016/10/11 12:55:35, machenbach (slow) wrote: > > > I hope this actually switches it on. The gn defaults add > > > -Wno-inconsistent-missing-override and we append this afterwards. > > > > A local test confirms that it should be switched on by this change. > > This works by luck I think. The usual way is to put warning flags in a config > (https://cs.chromium.org/chromium/src/third_party/mesa/BUILD.gn?rcl=0&l=119) This is a config.
The CQ bit was checked by machenbach@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [gn] Add back two warnings V8 uses with gyp BUG=428099 ========== to ========== [gn] Add back two warnings V8 uses with gyp BUG=428099 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [gn] Add back two warnings V8 uses with gyp BUG=428099 ========== to ========== [gn] Add back two warnings V8 uses with gyp BUG=428099 Committed: https://crrev.com/f18a9ad780a71121c2e2cb98d7a869e5b5b38478 Cr-Commit-Position: refs/heads/master@{#40235} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/f18a9ad780a71121c2e2cb98d7a869e5b5b38478 Cr-Commit-Position: refs/heads/master@{#40235} |
