Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(177)

Issue 10827429: Turn on warnings as errors for (most) third_party code on Windows (Closed)

Created:
8 years, 4 months ago by scottmg
Modified:
8 years, 3 months ago
Reviewers:
Nico
CC:
chromium-reviews
Visibility:
Public.

Description

Turn on warnings as errors for (most) third_party code on Windows Can't quite be fully enabled yet due to not-yet-fixed third party dependencies. Without it enabled, other packages regress while we're fixing things. So, add a flag for now so warnings-freeness can be ratcheted forward by having it on for most packages, but off for a few. Also, disable warning in qcms (fixed upstream by a large refactoring, not worth rolling for), and disable two minor warnings in yasm (patch posted upstream for a few months, but maintainer does not seem motivated to merge). Fix release-only warning in leveldatabase/env_chromium.cc. Was calling exit(1) in a leaky destructor. Fix a warning in lzma_sdk (missing an include). Disable a silly warning in Release builds of skia and memory_watcher (that /GS is not working because optimization is disabled). Warning are currently tolerated in libvpx and mesa. Cannot be committed until http://chromiumcodereview.appspot.com/10823426/ has landed. R=thakis@chromium.org BUG=126483, 140121, 143877 TEST= Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=153092

Patch Set 1 #

Total comments: 4

Patch Set 2 : add comments #

Patch Set 3 : add deps roll for libvpx #

Patch Set 4 : fix release-only warning in leveldatabase/env_chromium.cc #

Patch Set 5 : allow warnings in mesa #

Patch Set 6 : skia #

Patch Set 7 : another in tools/memory_watcher #

Patch Set 8 : lzma_sdk #

Unified diffs Side-by-side diffs Delta from patch set Stats (+29 lines, -5 lines) Patch
M DEPS View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M build/common.gypi View 1 2 3 4 5 2 chunks +9 lines, -1 line 0 comments Download
M skia/skia.gyp View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M third_party/leveldatabase/env_chromium.cc View 1 2 3 1 chunk +1 line, -2 lines 0 comments Download
M third_party/lzma_sdk/CpuArch.h View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M third_party/lzma_sdk/README.chromium View 1 2 3 4 5 6 7 1 chunk +3 lines, -0 lines 0 comments Download
M third_party/mesa/mesa.gyp View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
M third_party/qcms/qcms.gyp View 1 1 chunk +3 lines, -0 lines 0 comments Download
M third_party/yasm/yasm.gyp View 1 1 chunk +3 lines, -0 lines 0 comments Download
M tools/memory_watcher/memory_watcher.gyp View 1 2 3 4 5 6 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 10 (0 generated)
scottmg
8 years, 4 months ago (2012-08-20 23:40:24 UTC) #1
Nico
lgtm https://chromiumcodereview.appspot.com/10827429/diff/1/third_party/qcms/qcms.gyp File third_party/qcms/qcms.gyp (right): https://chromiumcodereview.appspot.com/10827429/diff/1/third_party/qcms/qcms.gyp#newcode29 third_party/qcms/qcms.gyp:29: 'msvs_disabled_warnings': [ 4018 ], nit: explanatory comment? https://chromiumcodereview.appspot.com/10827429/diff/1/third_party/yasm/yasm.gyp ...
8 years, 4 months ago (2012-08-20 23:43:17 UTC) #2
scottmg
https://chromiumcodereview.appspot.com/10827429/diff/1/third_party/qcms/qcms.gyp File third_party/qcms/qcms.gyp (right): https://chromiumcodereview.appspot.com/10827429/diff/1/third_party/qcms/qcms.gyp#newcode29 third_party/qcms/qcms.gyp:29: 'msvs_disabled_warnings': [ 4018 ], On 2012/08/20 23:43:17, Nico wrote: ...
8 years, 4 months ago (2012-08-20 23:47:46 UTC) #3
scottmg
Oh, I should include the DEPS roll here for libvpx too so that it's mostly ...
8 years, 4 months ago (2012-08-20 23:50:00 UTC) #4
scottmg
fyi, couple more changes in patchset 4+5.
8 years, 4 months ago (2012-08-21 16:00:22 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scottmg@chromium.org/10827429/13001
8 years, 4 months ago (2012-08-21 16:20:41 UTC) #6
scottmg
On 2012/08/21 16:20:41, I haz the power (commit-bot) wrote: > CQ is trying da patch. ...
8 years, 4 months ago (2012-08-21 17:37:11 UTC) #7
Nico
still lgtm
8 years, 4 months ago (2012-08-21 17:49:08 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scottmg@chromium.org/10827429/15004
8 years, 4 months ago (2012-08-23 18:36:08 UTC) #9
commit-bot: I haz the power
8 years, 4 months ago (2012-08-23 22:32:26 UTC) #10
Change committed as 153092

Powered by Google App Engine
This is Rietveld 408576698