win clang: don't disable -Wself-assign

win clang: don't disable -Wself-assign

R=thakis@chromium.org
TBR=brettw@chromium.org
BUG=505306
Committed: https://crrev.com/838cea57c725da43f9899fee9235fa4fb35bccda
Cr-Commit-Position: refs/heads/master@{#336820}

[scottmg, 2015-06-29 22:38:24]

The CQ bit was checked by scottmg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-29 22:38:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1218903002/20001

[commit-bot: I haz the power, 2015-06-29 22:44:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-29 22:44:40]

Dry run: Exceeded global retry quota

[Nico, 2015-06-29 23:51:02]

Thanks! A few small comments:

https://codereview.chromium.org/1218903002/diff/40001/third_party/libpng/BUIL...
File third_party/libpng/BUILD.gn (right):

https://codereview.chromium.org/1218903002/diff/40001/third_party/libpng/BUIL...
third_party/libpng/BUILD.gn:66: cflags = [ "-Wno-self-assign" ]  #
http://crbug.com/505306.
Is the plan to fix these? If not and this is supposed to stay here, instead add
a comment what it's needed for. (And it should probably be only `if (is_clang)`
– right now we happen to build third-party code with -Wall with clang-cl but not
with regular clang, but since everything's close to building in that mode we
might as well enable -Wall for third-party code soon too)

https://codereview.chromium.org/1218903002/diff/40001/third_party/libpng/libp...
File third_party/libpng/libpng.gyp (right):

https://codereview.chromium.org/1218903002/diff/40001/third_party/libpng/libp...
third_party/libpng/libpng.gyp:76: '-Wno-self-assign',  #
http://crbug.com/505306.
Likewise – add a

  'variables': {
    'clang_warning_flags': [ '-Wno-self-assign' ]
  }

block to the target instead

https://codereview.chromium.org/1218903002/diff/40001/third_party/lzma_sdk/7z...
File third_party/lzma_sdk/7zAlloc.c (left):

https://codereview.chromium.org/1218903002/diff/40001/third_party/lzma_sdk/7z...
third_party/lzma_sdk/7zAlloc.c:23: p = p;
I bet these are here to fix -Wunused-parameter warnings (which we don't enable).
If you want to upstream this, you probably want to remove p's name
(`SzAlloc(void*, size_t size)`) or change this line to `(void)p;`

https://codereview.chromium.org/1218903002/diff/40001/third_party/lzma_sdk/Lz...
File third_party/lzma_sdk/LzmaEnc.c (left):

https://codereview.chromium.org/1218903002/diff/40001/third_party/lzma_sdk/Lz...
third_party/lzma_sdk/LzmaEnc.c:2081: pp = pp;
likewise, (void)pp

https://codereview.chromium.org/1218903002/diff/40001/third_party/lzma_sdk/Lz...
File third_party/lzma_sdk/LzmaLib.c (right):

https://codereview.chromium.org/1218903002/diff/40001/third_party/lzma_sdk/Lz...
third_party/lzma_sdk/LzmaLib.c:11: static void *SzAlloc(void *p, size_t size) {
return MyAlloc(size); }
likewise, remove name of 1st param

[scottmg, 2015-06-30 00:30:59]

Patchset #4 (id:60001) has been deleted

[scottmg, 2015-06-30 00:32:55]

https://codereview.chromium.org/1218903002/diff/40001/third_party/libpng/BUIL...
File third_party/libpng/BUILD.gn (right):

https://codereview.chromium.org/1218903002/diff/40001/third_party/libpng/BUIL...
third_party/libpng/BUILD.gn:66: cflags = [ "-Wno-self-assign" ]  #
http://crbug.com/505306.
On 2015/06/29 23:51:02, Nico wrote:
> Is the plan to fix these? If not and this is supposed to stay here, instead
add
> a comment what it's needed for. (And it should probably be only `if
(is_clang)`
> – right now we happen to build third-party code with -Wall with clang-cl
but not
> with regular clang, but since everything's close to building in that mode we
> might as well enable -Wall for third-party code soon too)

Done.

https://codereview.chromium.org/1218903002/diff/40001/third_party/libpng/libp...
File third_party/libpng/libpng.gyp (right):

https://codereview.chromium.org/1218903002/diff/40001/third_party/libpng/libp...
third_party/libpng/libpng.gyp:76: '-Wno-self-assign',  #
http://crbug.com/505306.
On 2015/06/29 23:51:02, Nico wrote:
> Likewise – add a
> 
>   'variables': {
>     'clang_warning_flags': [ '-Wno-self-assign' ]
>   }
> 
> block to the target instead

Done.

https://codereview.chromium.org/1218903002/diff/40001/third_party/lzma_sdk/7z...
File third_party/lzma_sdk/7zAlloc.c (left):

https://codereview.chromium.org/1218903002/diff/40001/third_party/lzma_sdk/7z...
third_party/lzma_sdk/7zAlloc.c:23: p = p;
On 2015/06/29 23:51:02, Nico wrote:
> I bet these are here to fix -Wunused-parameter warnings (which we don't
enable).
> If you want to upstream this, you probably want to remove p's name
> (`SzAlloc(void*, size_t size)`) or change this line to `(void)p;`

I don't think you can remove the argument names in C? (At least you can't in
VS).

Changed to disabling the warning locally rather than modifying the code as it
doesn't seem like a worthwhile enough change to bug upstream about.

[scottmg, 2015-06-30 00:33:08]

The CQ bit was checked by scottmg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-30 00:35:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1218903002/80001

[Nico, 2015-06-30 00:35:26]

lgtm

[commit-bot: I haz the power, 2015-06-30 02:16:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-30 02:16:54]

Dry run: This issue passed the CQ dry run.

[Nico, 2015-06-30 04:39:54]

Prompted by mgiuca's chromium-dev post: Did you try this with gn? (I suppose the
bots will catch this if it doesn't work there, worst-case)

[scottmg, 2015-06-30 16:13:40]

On 2015/06/30 04:39:54, Nico wrote:
> Prompted by mgiuca's chromium-dev post: Did you try this with gn? (I suppose
the
> bots will catch this if it doesn't work there, worst-case)

I didn't, but at least there's win8_chromium_ng for non-clang gn here.

[scottmg, 2015-06-30 16:13:47]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2015-06-30 16:14:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1218903002/80001

[commit-bot: I haz the power, 2015-06-30 16:21:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-30 16:21:24]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[scottmg, 2015-06-30 16:24:41]

On 2015/06/30 16:13:40, scottmg (ooo jul1 to aug6) wrote:
> On 2015/06/30 04:39:54, Nico wrote:
> > Prompted by mgiuca's chromium-dev post: Did you try this with gn? (I suppose
> the
> > bots will catch this if it doesn't work there, worst-case)
> 
> I didn't, but at least there's win8_chromium_ng for non-clang gn here.

Oh, I see mgiuca's post now. Yeah, this probably doesn't work, bleh.

[scottmg, 2015-06-30 16:33:54]

On 2015/06/30 16:24:41, scottmg (ooo jul1 to aug6) wrote:
> On 2015/06/30 16:13:40, scottmg (ooo jul1 to aug6) wrote:
> > On 2015/06/30 04:39:54, Nico wrote:
> > > Prompted by mgiuca's chromium-dev post: Did you try this with gn? (I
suppose
> > the
> > > bots will catch this if it doesn't work there, worst-case)
> > 
> > I didn't, but at least there's win8_chromium_ng for non-clang gn here.
> 
> Oh, I see mgiuca's post now. Yeah, this probably doesn't work, bleh.

Now in a local config block for gn warnings.

[scottmg, 2015-06-30 16:35:15]

scottmg@chromium.org changed reviewers:
+ brettw@chromium.org

[scottmg, 2015-06-30 16:35:16]

tbr brettw for third_party/libpng and third_party/lzma_sdk build file changes.

[scottmg, 2015-06-30 16:35:20]

The CQ bit was checked by scottmg@chromium.org

[scottmg, 2015-06-30 16:35:21]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1218903002/#ps100001
(title: "fix lzma_sdk and libpng for gn build")

[commit-bot: I haz the power, 2015-06-30 16:35:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1218903002/100001

[commit-bot: I haz the power, 2015-06-30 18:10:43]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-06-30 18:11:35]

Patchset 5 (id:??) landed as
https://crrev.com/838cea57c725da43f9899fee9235fa4fb35bccda
Cr-Commit-Position: refs/heads/master@{#336820}