Suppress warnings for assignment inside conditionals.

Suppress warnings for assignment inside conditionals.

Some Windows-specific ICU code uses this pattern, which triggers a clang warning.

BUG=505302
R=thakis@chromium.org

Committed: https://chromium.googlesource.com/chromium/deps/icu/+/1b697da5c2c0112e2b70e7e75d3e3d985f464a8f

[dcheng, 2015-06-30 04:04:06]

dcheng@chromium.org changed reviewers:
+ thakis@chromium.org

[dcheng, 2015-06-30 04:04:06]

The alternative is to try to patch this upstream
(http://source.icu-project.org/repos/icu/icu/trunk/source/common/putil.cpp,
search for p = uprv).

[Nico, 2015-06-30 04:45:56]

I think disabling this is fine, but there should be a short comment explaining
why, like in the clang_warning_flags section (`# ICU uses code like `while(p =
uprv_strch(...))` without parens around the assignment` or similar, short enough
to fit on one line).

lgtm with the commend and with adding the flag everywhere, not just on windows

https://codereview.chromium.org/1209413007/diff/20001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1209413007/diff/20001/BUILD.gn#newcode80
BUILD.gn:80: "-Wno-parentheses",
I'd probably put this in the general is_clang block above, there isn't much
Windows-specific about this. (It's currently only needed on Windows because we
happen to build third-party code with -Wall on Windows but not elsewhere, but
that's mostly by accident.)

https://codereview.chromium.org/1209413007/diff/20001/icu.gyp
File icu.gyp (right):

https://codereview.chromium.org/1209413007/diff/20001/icu.gyp#newcode239
icu.gyp:239: # clang_warning_flags block above.
See this.

[Nico, 2015-06-30 04:46:02]

*comment

[dcheng, 2015-06-30 17:42:16]

PTAL.

I fixed an oversight in the previous patch (there are two distinct warning flag
blocks for icu.gyp). For one of them, the warnings looked Windows-specific (they
were all in windtfmt.cpp, which is blocked out by #if U_PLATFORM_HAS_WIN32_API).
For the other, I moved it to clang_warning_flags per your suggestion
(originally, I thought it was Win32-specific too, but I found some other
instances in putil.cpp that would trigger on non-Windows platforms).

[Nico, 2015-06-30 18:12:10]

lgtm, but lebtm with the .gyp comment below

https://codereview.chromium.org/1209413007/diff/40001/icu.gyp
File icu.gyp (right):

https://codereview.chromium.org/1209413007/diff/40001/icu.gyp#newcode295
icu.gyp:295: '-Wno-parentheses',
So this is Win-only in icui18n but global in icuuc? That's a bit strange, but ok
I guess. Doesn't match gn though. Maybe put it in clang_warning_flags for both
targets and remove the is_win in the gn file for consistency / simplicity?

[dcheng, 2015-06-30 21:18:09]

Mind taking one more look? I'm not as familiar with the GN-side of things, and I
found what looks like an oversight in the current BUILD.gn for it: I wanted to
make sure that I fixed it the right way.

(It looks like we might be able to trim down the list of disabled warnings,
since I only saw -Wparentheses related warnings, but that's probably a yak to
shave another day...)

https://codereview.chromium.org/1209413007/diff/60001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1209413007/diff/60001/BUILD.gn#newcode495
BUILD.gn:495: configs += [ ":icu_code" ]
Apparently it wasn't really suppressing the warnings before. It is now =)

[Nico, 2015-06-30 21:32:58]

lgtm!

https://codereview.chromium.org/1209413007/diff/60001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1209413007/diff/60001/BUILD.gn#newcode495
BUILD.gn:495: configs += [ ":icu_code" ]
On 2015/06/30 21:18:09, dcheng wrote:
> Apparently it wasn't really suppressing the warnings before. It is now =)

Ah cool. Yes, the += appends no_chromium_code to configs which adds -Wall, so
the compile command would end up with

  "-Wno-foo -Wall"

and Wfoo was part of Wall so Wall reenabled it. Now the -Wno-foo is passed after
-Wall.

This transformation isn't safe in general: If icu_code added include
directories, you'd want the icu_code directories to come in front of the general
include directories. Since it only contains defines and warning flags, this is a
good change however.

[dcheng, 2015-06-30 22:06:05]

Committed patchset #4 (id:60001) manually as
1b697da5c2c0112e2b70e7e75d3e3d985f464a8f (presubmit successful).