Enforce WARN_UNUSED_RESULT attribute on Windows

Enforce WARN_UNUSED_RESULT attribute on Windows

BUG=none
R=cpu,danakj,dpranke,hans,kelvinp,rickyz,sky,sorin,thestig,tuttle,yukawa
TBR=vitalybuka

Committed: https://crrev.com/7764fe9ea52233096b7916614eb7c60b26bbca05
Cr-Commit-Position: refs/heads/master@{#353450}

[dcheng, 2015-10-07 16:02:25]

dcheng@chromium.org changed reviewers:
+ danakj@chromium.org, rickyz@chromium.org

[dcheng, 2015-10-07 16:03:20]

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

[dcheng, 2015-10-07 16:03:56]

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

[dcheng, 2015-10-07 16:04:19]

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

[dcheng, 2015-10-07 16:05:07]

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

[dcheng, 2015-10-07 16:05:39]

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

[dcheng, 2015-10-07 16:06:20]

dcheng@chromium.org changed reviewers:
+ hans@chromium.org, yukawa@chromium.org

[dcheng, 2015-10-07 16:06:32]

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

[commit-bot: I haz the power, 2015-10-07 16:09:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1390223002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1390223002/1

[dcheng, 2015-10-07 16:09:59]

Since we have win clang bots now, we can 

+danakj@ for base/ changes
+hans@ for build/ flag changes
+vitalybuka@ for cloud_print/ changes
+kelvinp@ for remoting/ changes
+rdsmith@ for net/ changes
+rickyz@ for sandbox/ changes
+sky@ for ui/ changes
+sorin@ for components/update_client/ changes
+thestig@ for chrome/, components/storage_monitor changes
+yukawa@ for media/midi changes.

https://codereview.chromium.org/1390223002/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1390223002/diff/1/build/common.gypi#newcode5767
build/common.gypi:5767: '-Wunused-result',
/W4 doesn't seem to enable this warning, so I had to manually add it to this
list.

[dcheng, 2015-10-07 16:10:36]

On 2015/10/07 at 16:09:59, dcheng wrote:
> Since we have win clang bots now, we can 

... actually make this checked by the compiler and fix the (minor) issues that
it caught.

> 
> +danakj@ for base/ changes
> +hans@ for build/ flag changes
> +vitalybuka@ for cloud_print/ changes
> +kelvinp@ for remoting/ changes
> +rdsmith@ for net/ changes
> +rickyz@ for sandbox/ changes
> +sky@ for ui/ changes
> +sorin@ for components/update_client/ changes
> +thestig@ for chrome/, components/storage_monitor changes
> +yukawa@ for media/midi changes.
> 
> https://codereview.chromium.org/1390223002/diff/1/build/common.gypi
> File build/common.gypi (right):
> 
>
https://codereview.chromium.org/1390223002/diff/1/build/common.gypi#newcode5767
> build/common.gypi:5767: '-Wunused-result',
> /W4 doesn't seem to enable this warning, so I had to manually add it to this
list.

[danakj, 2015-10-07 16:16:54]

https://codereview.chromium.org/1390223002/diff/1/base/compiler_specific.h
File base/compiler_specific.h (right):

https://codereview.chromium.org/1390223002/diff/1/base/compiler_specific.h#ne...
base/compiler_specific.h:122: // TODO(dcheng): Update //third_party/webrtc's
macro definition to match.
Could we ifdef then undef and redefine here?

[hans, 2015-10-07 16:25:26]

https://codereview.chromium.org/1390223002/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1390223002/diff/1/build/common.gypi#newcode5767
build/common.gypi:5767: '-Wunused-result',
On 2015/10/07 16:09:59, dcheng wrote:
> /W4 doesn't seem to enable this warning, so I had to manually add it to this
> list.

Hmm, clang-cl /W4 does enable it for me. Can you double check? That we're
disabling -Wunused-value above suggests that these warnings are on by default.

Also, this is a list of disabled warnings (see the comment about making it
shorter above). If we need to enable this explicitly, let's move it to
clang_warning_flags above.

https://codereview.chromium.org/1390223002/diff/1/build/config/compiler/BUILD.gn
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/1390223002/diff/1/build/config/compiler/BUILD...
build/config/compiler/BUILD.gn:579: "-Wunused-result",
(See the common.gypi comment.)

[Lei Zhang, 2015-10-07 16:28:09]

https://codereview.chromium.org/1390223002/diff/1/chrome/installer/util/delet...
File chrome/installer/util/delete_tree_work_item.cc (left):

https://codereview.chromium.org/1390223002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_tree_work_item.cc:70: backup.Delete();
Why not Delete() ?

https://codereview.chromium.org/1390223002/diff/1/chrome/utility/importer/ie_...
File chrome/utility/importer/ie_importer_win.cc (right):

https://codereview.chromium.org/1390223002/diff/1/chrome/utility/importer/ie_...
chrome/utility/importer/ie_importer_win.cc:163: // GCC, in its infinite wisdom,
does not allow WARN_UNUSED_RESULT to be used in
What is GCC doing touching a _win.cc file? Did you mean Clang? Can we fix Clang
if that's what you meant?

https://codereview.chromium.org/1390223002/diff/1/components/storage_monitor....
File components/storage_monitor.gypi (right):

https://codereview.chromium.org/1390223002/diff/1/components/storage_monitor....
components/storage_monitor.gypi:123: }, 'OS=="win"', {
Oh, does gyp support this new syntax now?

[dcheng, 2015-10-07 16:41:34]

https://codereview.chromium.org/1390223002/diff/1/base/compiler_specific.h
File base/compiler_specific.h (right):

https://codereview.chromium.org/1390223002/diff/1/base/compiler_specific.h#ne...
base/compiler_specific.h:122: // TODO(dcheng): Update //third_party/webrtc's
macro definition to match.
On 2015/10/07 at 16:16:54, danakj_OOO_til_10-12 wrote:
> Could we ifdef then undef and redefine here?

Done (I just unconditionally undef since this file has an include guard anyway)

https://codereview.chromium.org/1390223002/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1390223002/diff/1/build/common.gypi#newcode5767
build/common.gypi:5767: '-Wunused-result',
On 2015/10/07 at 16:25:26, hans wrote:
> On 2015/10/07 16:09:59, dcheng wrote:
> > /W4 doesn't seem to enable this warning, so I had to manually add it to this
> > list.
> 
> Hmm, clang-cl /W4 does enable it for me. Can you double check? That we're
disabling -Wunused-value above suggests that these warnings are on by default.
> 
> Also, this is a list of disabled warnings (see the comment about making it
shorter above). If we need to enable this explicitly, let's move it to
clang_warning_flags above.

Hm...

Well it's definitely not enabled by default on the Chrome build, but I have no
idea what flag is causing it yet.

(It's easy enough to prove this by patching in just the compiler_specific.h
changes and then introducing an intentional error in base_unittests somewhere)

https://codereview.chromium.org/1390223002/diff/1/chrome/installer/util/delet...
File chrome/installer/util/delete_tree_work_item.cc (left):

https://codereview.chromium.org/1390223002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_tree_work_item.cc:70: backup.Delete();
On 2015/10/07 at 16:28:09, Lei Zhang wrote:
> Why not Delete() ?

It's a ScopedTmpDir, so it self-deletes anyway when it exits the scope (which
already emits a warning message if the delete fails: see
https://code.google.com/p/chromium/codesearch#chromium/src/base/files/scoped_...)

https://codereview.chromium.org/1390223002/diff/1/chrome/utility/importer/ie_...
File chrome/utility/importer/ie_importer_win.cc (right):

https://codereview.chromium.org/1390223002/diff/1/chrome/utility/importer/ie_...
chrome/utility/importer/ie_importer_win.cc:163: // GCC, in its infinite wisdom,
does not allow WARN_UNUSED_RESULT to be used in
On 2015/10/07 at 16:28:09, Lei Zhang wrote:
> What is GCC doing touching a _win.cc file? Did you mean Clang? Can we fix
Clang if that's what you meant?

GCC only allows function attributes on prototypes.
Therefore, clang emits a warning for GCC compatibility.
I'm honestly not sure what the best thing to say here is: I guess we could have
clang-cl implicitly disable -Wgcc-compat

https://codereview.chromium.org/1390223002/diff/1/components/storage_monitor....
File components/storage_monitor.gypi (right):

https://codereview.chromium.org/1390223002/diff/1/components/storage_monitor....
components/storage_monitor.gypi:123: }, 'OS=="win"', {
On 2015/10/07 at 16:28:09, Lei Zhang wrote:
> Oh, does gyp support this new syntax now?

Huh, oops. Fixed (I'm surprised this gyped successfully)

[hans, 2015-10-07 16:51:29]

https://codereview.chromium.org/1390223002/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1390223002/diff/1/build/common.gypi#newcode5767
build/common.gypi:5767: '-Wunused-result',
On 2015/10/07 16:41:34, dcheng wrote:
> On 2015/10/07 at 16:25:26, hans wrote:
> > On 2015/10/07 16:09:59, dcheng wrote:
> > > /W4 doesn't seem to enable this warning, so I had to manually add it to
this
> > > list.
> > 
> > Hmm, clang-cl /W4 does enable it for me. Can you double check? That we're
> disabling -Wunused-value above suggests that these warnings are on by default.
> > 
> > Also, this is a list of disabled warnings (see the comment about making it
> shorter above). If we need to enable this explicitly, let's move it to
> clang_warning_flags above.
> 
> Hm...
> 
> Well it's definitely not enabled by default on the Chrome build, but I have no
> idea what flag is causing it yet.
> 
> (It's easy enough to prove this by patching in just the compiler_specific.h
> changes and then introducing an intentional error in base_unittests somewhere)

D'oh, it's right there: -Wno-unused-value also disables the unused-result
warning. Sorry, I should have noticed that.

Maybe something like:

  # Disable unused-value (crbug.com/505318) except -Wunused-result.
  '-Wno-unused-value',
  '-Wunused-result',

[dcheng, 2015-10-07 17:46:15]

https://codereview.chromium.org/1390223002/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1390223002/diff/1/build/common.gypi#newcode5767
build/common.gypi:5767: '-Wunused-result',
On 2015/10/07 at 16:51:29, hans wrote:
> On 2015/10/07 16:41:34, dcheng wrote:
> > On 2015/10/07 at 16:25:26, hans wrote:
> > > On 2015/10/07 16:09:59, dcheng wrote:
> > > > /W4 doesn't seem to enable this warning, so I had to manually add it to
this
> > > > list.
> > > 
> > > Hmm, clang-cl /W4 does enable it for me. Can you double check? That we're
> > disabling -Wunused-value above suggests that these warnings are on by
default.
> > > 
> > > Also, this is a list of disabled warnings (see the comment about making it
> > shorter above). If we need to enable this explicitly, let's move it to
> > clang_warning_flags above.
> > 
> > Hm...
> > 
> > Well it's definitely not enabled by default on the Chrome build, but I have
no
> > idea what flag is causing it yet.
> > 
> > (It's easy enough to prove this by patching in just the compiler_specific.h
> > changes and then introducing an intentional error in base_unittests
somewhere)
> 
> D'oh, it's right there: -Wno-unused-value also disables the unused-result
warning. Sorry, I should have noticed that.
> 
> Maybe something like:
> 
>   # Disable unused-value (crbug.com/505318) except -Wunused-result.
>   '-Wno-unused-value',
>   '-Wunused-result',

Done.

[hans, 2015-10-07 17:48:10]

build/ flag changes: lgtm

[sky, 2015-10-07 17:55:07]

LGTM

[Sorin Jianu, 2015-10-07 18:12:17]

lgtm

thank you!

[Deprecated (see juliatuttle), 2015-10-07 18:25:41]

ttuttle@chromium.org changed reviewers:
+ ttuttle@chromium.org

[Deprecated (see juliatuttle), 2015-10-07 18:25:42]

net/dns lgtm with nit

https://codereview.chromium.org/1390223002/diff/60001/net/dns/address_sorter_...
File net/dns/address_sorter_win.cc (right):

https://codereview.chromium.org/1390223002/diff/60001/net/dns/address_sorter_...
net/dns/address_sorter_win.cc:115: DCHECK(result) << "Unable to roundtrip
between IPEndPoint and "
Perhaps also continue; the loop if result is false, since ipe will be empty
anyway?

[kelvinp, 2015-10-07 18:32:58]

lgtm

[danakj, 2015-10-07 18:33:59]

base LGTM

[dcheng, 2015-10-07 18:52:01]

https://codereview.chromium.org/1390223002/diff/60001/net/dns/address_sorter_...
File net/dns/address_sorter_win.cc (right):

https://codereview.chromium.org/1390223002/diff/60001/net/dns/address_sorter_...
net/dns/address_sorter_win.cc:115: DCHECK(result) << "Unable to roundtrip
between IPEndPoint and "
On 2015/10/07 at 18:25:41, ttuttle wrote:
> Perhaps also continue; the loop if result is false, since ipe will be empty
anyway?

Basically, as I see it, if this ever fails, something already went really wrong:
either way, the result is broken. There are either empty IPEndpoints, or the
sorted list is somehow shorter.

Thus, I think the right thing to do is just DCHECK, since this "should never
happen".

[yukawa, 2015-10-07 20:08:22]

media/midi/midi_manager_win.cc: lgtm.

[Lei Zhang, 2015-10-07 20:33:11]

https://codereview.chromium.org/1390223002/diff/1/chrome/installer/util/delet...
File chrome/installer/util/delete_tree_work_item.cc (left):

https://codereview.chromium.org/1390223002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_tree_work_item.cc:70: backup.Delete();
On 2015/10/07 16:41:34, dcheng wrote:
> On 2015/10/07 at 16:28:09, Lei Zhang wrote:
> > Why not Delete() ?
> 
> It's a ScopedTmpDir, so it self-deletes anyway when it exits the scope (which
> already emits a warning message if the delete fails: see
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/files/scoped_...)

But |backup| is a reference to |key_backup_paths_[i]|.

https://codereview.chromium.org/1390223002/diff/1/chrome/utility/importer/ie_...
File chrome/utility/importer/ie_importer_win.cc (right):

https://codereview.chromium.org/1390223002/diff/1/chrome/utility/importer/ie_...
chrome/utility/importer/ie_importer_win.cc:163: // GCC, in its infinite wisdom,
does not allow WARN_UNUSED_RESULT to be used in
On 2015/10/07 16:41:34, dcheng wrote:
> On 2015/10/07 at 16:28:09, Lei Zhang wrote:
> > What is GCC doing touching a _win.cc file? Did you mean Clang? Can we fix
> Clang if that's what you meant?
> 
> GCC only allows function attributes on prototypes.
> Therefore, clang emits a warning for GCC compatibility.
> I'm honestly not sure what the best thing to say here is: I guess we could
have
> clang-cl implicitly disable -Wgcc-compat

Can you mention the clang / gcc compatibility issue? Otherwise it's confusing
for readers.

https://codereview.chromium.org/1390223002/diff/1/components/storage_monitor....
File components/storage_monitor.gypi (right):

https://codereview.chromium.org/1390223002/diff/1/components/storage_monitor....
components/storage_monitor.gypi:123: }, 'OS=="win"', {
On 2015/10/07 16:41:34, dcheng wrote:
> On 2015/10/07 at 16:28:09, Lei Zhang wrote:
> > Oh, does gyp support this new syntax now?
> 
> Huh, oops. Fixed (I'm surprised this gyped successfully)

It might actually be valid, but even if it is, it's not commonly used.

[rickyz (no longer on Chrome), 2015-10-07 21:23:24]

rickyz@chromium.org changed reviewers:
+ cpu@chromium.org

[rickyz (no longer on Chrome), 2015-10-07 21:23:24]

The sandbox/ changes lgtm, adding cpu@ who's an OWNER.

[dcheng, 2015-10-08 03:48:07]

https://codereview.chromium.org/1390223002/diff/1/chrome/installer/util/delet...
File chrome/installer/util/delete_tree_work_item.cc (left):

https://codereview.chromium.org/1390223002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_tree_work_item.cc:70: backup.Delete();
On 2015/10/07 at 20:33:11, Lei Zhang wrote:
> On 2015/10/07 16:41:34, dcheng wrote:
> > On 2015/10/07 at 16:28:09, Lei Zhang wrote:
> > > Why not Delete() ?
> > 
> > It's a ScopedTmpDir, so it self-deletes anyway when it exits the scope
(which
> > already emits a warning message if the delete fails: see
> >
https://code.google.com/p/chromium/codesearch#chromium/src/base/files/scoped_...)
> 
> But |backup| is a reference to |key_backup_paths_[i]|.

Done. Thanks for pointing this out.

https://codereview.chromium.org/1390223002/diff/1/chrome/utility/importer/ie_...
File chrome/utility/importer/ie_importer_win.cc (right):

https://codereview.chromium.org/1390223002/diff/1/chrome/utility/importer/ie_...
chrome/utility/importer/ie_importer_win.cc:163: // GCC, in its infinite wisdom,
does not allow WARN_UNUSED_RESULT to be used in
On 2015/10/07 at 20:33:11, Lei Zhang wrote:
> On 2015/10/07 16:41:34, dcheng wrote:
> > On 2015/10/07 at 16:28:09, Lei Zhang wrote:
> > > What is GCC doing touching a _win.cc file? Did you mean Clang? Can we fix
> > Clang if that's what you meant?
> > 
> > GCC only allows function attributes on prototypes.
> > Therefore, clang emits a warning for GCC compatibility.
> > I'm honestly not sure what the best thing to say here is: I guess we could
have
> > clang-cl implicitly disable -Wgcc-compat
> 
> Can you mention the clang / gcc compatibility issue? Otherwise it's confusing
for readers.

Done.

[Lei Zhang, 2015-10-08 03:53:14]

lgtm

[Randy Smith (Not in Mondays), 2015-10-08 19:08:55]

rdsmith@chromium.org changed reviewers:
- rdsmith@chromium.org

[dcheng, 2015-10-08 19:09:51]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2015-10-08 19:09:51]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
hans@chromium.org, yukawa@chromium.org, danakj@chromium.org, sorin@chromium.org,
rickyz@chromium.org, ttuttle@chromium.org, kelvinp@chromium.org
Link to the patchset: https://codereview.chromium.org/1390223002/#ps80001
(title: "Address comments")

[commit-bot: I haz the power, 2015-10-08 19:10:08]

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

[commit-bot: I haz the power, 2015-10-08 19:20:55]

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

[commit-bot: I haz the power, 2015-10-08 19:20:56]

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...)

[cpu_(ooo_6.6-7.5), 2015-10-08 20:08:14]

lgtm on sandbox

[dcheng, 2015-10-08 23:52:24]

dcheng@chromium.org changed reviewers:
+ agable@chromium.org, vitalybuka@chromium.org

[dcheng, 2015-10-08 23:52:25]

+agable for build changes, +vitalybuka for real for cloud_print

[agable, 2015-10-09 00:01:51]

agable@chromium.org changed reviewers:
+ dpranke@chromium.org

[agable, 2015-10-09 00:01:52]

-agable

I'm not actually in the /src/build/OWNERS file.

+dpranke instead

[Dirk Pranke, 2015-10-09 00:03:57]

build/ changes lgtm.

[dcheng, 2015-10-09 22:30:43]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2015-10-09 22:37:10]

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

[commit-bot: I haz the power, 2015-10-10 01:17:40]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-10-10 01:18:37]

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

[Vitaly Buka (NO REVIEWS), 2015-10-10 01:49:05]

lgtm