Adding V8_WARN_UNUSED_RESULT for specified TODOs

Adding V8_WARN_UNUSED_RESULT for specified TODOs

Currently there are a number of comment in src/v8.h which look like
this: TODO(dcarney): mark V8_WARN_UNUSED_RESULT.

This commit attempts to remove these comments and add the
V8_WARN_UNUSED_RESULT macro to the methods in question.

BUG=

Review-Url: https://codereview.chromium.org/2135973002
Cr-Commit-Position: refs/heads/master@{#44010}
Committed: https://chromium.googlesource.com/v8/v8/+/4e92e124858c56bd0c99533395c9c2333d954e5f

[danbev, 2016-07-11 13:33:02]

daniel.bevenius@gmail.com changed reviewers:
+ jochen@chromium.org

[danbev, 2016-07-11 13:33:03]

Would you mind giving this a review?

[jochen (gone - plz use gerrit), 2016-07-11 13:37:41]

have you checked that both chromium and pdfium are still compiling with this?

[danbev, 2016-07-11 13:41:37]

On 2016/07/11 13:37:41, jochen wrote:
> have you checked that both chromium and pdfium are still compiling with this?

Sorry, I haven't (not being lazy here, just a newbie mistake). Let me take a
look at them and report back.

[jochen (gone - plz use gerrit), 2016-07-11 13:44:05]

i'd probably first check in codesearch (cs.chromium.org) whether those methods
have any callers. If not, it's likely not a problem :)

[jochen (gone - plz use gerrit), 2016-07-11 13:44:25]

On 2016/07/11 at 13:44:05, jochen wrote:
> i'd probably first check in codesearch (cs.chromium.org) whether those methods
have any callers. If not, it's likely not a problem :)

ehrm, or, if there are callers, check that they all use the return value

[danbev, 2016-07-13 05:34:09]

On 2016/07/11 13:44:25, jochen wrote:
> On 2016/07/11 at 13:44:05, jochen wrote:
> > i'd probably first check in codesearch (http://cs.chromium.org) whether
those methods
> have any callers. If not, it's likely not a problem :)
> 
> ehrm, or, if there are callers, check that they all use the return value

Thanks for the pointers. I've tried compiling chromium and pdfium and did find a
number of places in chromium where the return value was not being used, but none
in pdfium. 

What I did was update the DEPS file to point to a branch on my github fork
(details can be found in the gist below). I tried adding this to the .gclient
custom_deps section, but this did not seem to work for me but I'll try to figure
out why this did not work as I think that is the proper way doing it. 

For the calls in chromium I've just added checks, but I was not sure about what
actions should be taken. This gist contains the diff:
https://gist.github.com/danbev/605643306e822ee3b8b010cf08fb17f4

Could you advice me on the best way to proceed with this?

[jochen (gone - plz use gerrit), 2016-07-13 08:57:45]

you'd upload a CL to chromium first, and once you've fixed all callsites, we can
land the V8 side change.

http://dev.chromium.org/developers/contributing-code has the documentation on
how to contribute to chromium.

let's go over all cases in that CL then and see what to do there

[danbev, 2016-07-21 16:50:18]

I've rebased and updated src/wasm/wasm-js.cc to use maybe variables and then use
CHECK to verify that they are not nothing, which is more inline with what is
being done in chromium.

[jochen (gone - plz use gerrit), 2016-07-22 12:22:32]

lgtm

[danbev, 2016-07-23 04:29:11]

The CQ bit was checked by daniel.bevenius@gmail.com

[commit-bot: I haz the power, 2016-07-23 04:29:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-23 04:32:08]

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

[commit-bot: I haz the power, 2016-07-23 04:32:09]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/20079)

[danbev, 2016-07-23 05:02:40]

daniel.bevenius@gmail.com changed reviewers:
+ ahaas@chromium.org
- jochen@chromium.org

[danbev, 2016-07-23 05:02:41]

Would you be able to review the changes in src/wasm/wasm-js.cc for me?

[danbev, 2016-07-26 10:51:22]

On 2016/07/22 12:22:32, jochen wrote:
> lgtm

Thanks. I'm just waiting for a lgtm from one of the owners of src/wasm:
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    src/wasm/wasm-js.cc

I've sent a request to ahaas@chromium.org (just the first in the list) but not
heard back yet. I did not want to annoy people by sending to everyone in the
owners file.
Should I just wait or try someone else as well?

[ahaas, 2016-07-26 13:20:50]

On 2016/07/26 at 10:51:22, daniel.bevenius wrote:
> On 2016/07/22 12:22:32, jochen wrote:
> > lgtm
> 
> Thanks. I'm just waiting for a lgtm from one of the owners of src/wasm:
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for these files:
>     src/wasm/wasm-js.cc
> 
> I've sent a request to ahaas@chromium.org (just the first in the list) but not
heard back yet. I did not want to annoy people by sending to everyone in the
owners file.
> Should I just wait or try someone else as well?

lgtm

[danbev, 2016-07-26 13:22:37]

On 2016/07/26 13:20:50, ahaas wrote:
> On 2016/07/26 at 10:51:22, daniel.bevenius wrote:
> > On 2016/07/22 12:22:32, jochen wrote:
> > > lgtm
> > 
> > Thanks. I'm just waiting for a lgtm from one of the owners of src/wasm:
> > ** Presubmit ERRORS **
> > Missing LGTM from an OWNER for these files:
> >     src/wasm/wasm-js.cc
> > 
> > I've sent a request to mailto:ahaas@chromium.org (just the first in the
list) but not
> heard back yet. I did not want to annoy people by sending to everyone in the
> owners file.
> > Should I just wait or try someone else as well?
> 
> lgtm

Thanks!

[danbev, 2016-07-26 13:23:18]

The CQ bit was checked by daniel.bevenius@gmail.com

[commit-bot: I haz the power, 2016-07-26 13:23:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-26 13:40:55]

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

[commit-bot: I haz the power, 2016-07-26 13:40:57]

Try jobs failed on following builders:
  v8_linux_chromium_gn_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_chromium_gn_rel/bu...)

[danbev, 2016-07-27 07:33:17]

On 2016/07/26 13:40:57, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   v8_linux_chromium_gn_rel on master.tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_chromium_gn_rel/bu...)

The failure reported is the following:

../../extensions/renderer/module_system.cc:460:5: error: ignoring return value
of function declared with 'warn_unused_result' attribute
[-Werror,-Wunused-result]
    object->Delete(context, property);
    ^~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~
../../extensions/renderer/module_system.cc:580:5: error: ignoring return value
of function declared with 'warn_unused_result' attribute
[-Werror,-Wunused-result]
    resolver->Reject(v8_context, v8::Exception::Error(ToV8StringUnsafe(
    ^~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../extensions/renderer/module_system.cc:755:3: error: ignoring return value
of function declared with 'warn_unused_result' attribute
[-Werror,-Wunused-result]
  resolver_local->Resolve(context()->v8_context(), value);
  ^~~~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
3 errors generated.

It looks like I've missed a step as the above is trying to compile chromium, but
for that to pass it will need to have the commit from
https://codereview.chromium.org/2144093002/. 
Could you guys help me and let me know how such a situation is normally handled?

[danbev, 2016-10-11 09:21:23]

Rebased with upstream/master

[danbev, 2016-10-11 11:21:21]

On 2016/10/11 09:21:23, danbev wrote:
> Rebased with upstream/master

[danbev, 2017-01-17 13:46:15]

daniel.bevenius@gmail.com changed reviewers:
+ jochen@chromium.org
- ahaas@chromium.org

[danbev, 2017-01-17 13:46:16]

I've rebased this with upstream/master. Would you mind taking a look? 

Thanks

[jochen (gone - plz use gerrit), 2017-01-17 13:57:06]

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

[commit-bot: I haz the power, 2017-01-17 13:57:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jochen (gone - plz use gerrit), 2017-01-17 13:57:14]

jochen@chromium.org changed reviewers:
+ ahaas@chromium.org

[jochen (gone - plz use gerrit), 2017-01-17 13:57:15]

i triggered a try run

[commit-bot: I haz the power, 2017-01-17 14:19:14]

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

[commit-bot: I haz the power, 2017-01-17 14:19:16]

Dry run: Try jobs failed on following builders:
  v8_linux_chromium_gn_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_chromium_gn_rel/bu...)

[danbev, 2017-01-25 06:42:14]

I've rebased this again. 

@jochen Thanks for running the try run. The error that was reported I think
looks like it comes when building chromium, by looking at:
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.v8%2Fv8_linux_...

I can see the following in the above output:
FAILED: obj/extensions/renderer/renderer/module_system.o 
/b/c/cipd/goma/gomacc ../../third_party/llvm-build/Release+Asserts/bin/clang++
-MMD -MF obj/extensions/renderer/renderer/module_system.o.d
-DV8_DEPRECATION_WARNINGS -DDCHECK_ALWAYS_ON=1 -DUSE_UDEV
-DUI_COMPOSITOR_IMAGE_TRANSPORT -DUSE_AURA=1 -DUSE_PANGO=1 -DUSE_CAIRO=1
-DUSE_GLIB=1 -DUSE_NSS_CERTS=1 -DUSE_X11=1 -DFULL_SAFE_BROWSING
-DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DCHROMIUM_BUILD
-DENABLE_MEDIA_ROUTER=1 -DFIELDTRIAL_TESTING_ENABLED
-DCR_CLANG_REVISION=289944-2 -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE
-D_LARGEFILE64_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DNDEBUG
-DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -DGL_GLEXT_PROTOTYPES -DUSE_GLX
-DUSE_EGL -DSK_IGNORE_DW_GRAY_FIX -DSK_IGNORE_LINEONLY_AA_CONVEX_PATH_OPTS
-DSK_SUPPORT_GPU=1 -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0
-DU_STATIC_IMPLEMENTATION -DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_FILE
-DGOOGLE_PROTOBUF_NO_RTTI -DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DHAVE_PTHREAD
-I../.. -Igen
-I../../build/linux/debian_wheezy_amd64-sysroot/usr/include/glib-2.0
-I../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/x86_64-linux-gnu/glib-2.0/include
-I../../third_party/khronos -I../../gpu -I../../third_party/libwebp
-Igen/extensions -Igen/extensions -Igen/extensions -Igen -I../../v8/include
-Igen/v8/include -I../../skia/config -I../../skia/ext
-I../../third_party/skia/include/c -I../../third_party/skia/include/config
-I../../third_party/skia/include/core -I../../third_party/skia/include/effects
-I../../third_party/skia/include/images -I../../third_party/skia/include/lazy
-I../../third_party/skia/include/pathops -I../../third_party/skia/include/pdf
-I../../third_party/skia/include/pipe -I../../third_party/skia/include/ports
-I../../third_party/skia/include/utils -I../../third_party/skia/include/gpu
-I../../third_party/skia/src/gpu -I../../third_party/skia/src/sksl
-I../../third_party/WebKit -Igen/third_party/WebKit -I../../v8/include
-Igen/v8/include -I../../third_party/icu/source/common
-I../../third_party/icu/source/i18n -I../../third_party/protobuf/src
-fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector -funwind-tables
-fPIC -pipe -B../../third_party/binutils/Linux_x64/Release/bin
-fcolor-diagnostics -fdebug-prefix-map=/b/c/b/v8_linux_chromium_gn_rel/src=.
-m64 -march=x86-64 -pthread -Wall -Werror -Wextra
-Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing
-Wno-covered-switch-default -Wno-deprecated-register
-Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override
-Wno-shift-negative-value -Wno-undefined-var-template
-Wno-nonportable-include-path -Wno-address-of-packed-member -O2 -fno-ident
-fdata-sections -ffunction-sections -g1
--sysroot=../../build/linux/debian_wheezy_amd64-sysroot -fvisibility=hidden
-Xclang -load -Xclang
../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so -Xclang
-add-plugin -Xclang find-bad-constructs -Xclang -plugin-arg-find-bad-constructs
-Xclang check-ipc -Wheader-hygiene -Wstring-conversion
-Wtautological-overlap-compare -fno-threadsafe-statics
-fvisibility-inlines-hidden -std=gnu++11 -fno-rtti -fno-exceptions -c
../../extensions/renderer/module_system.cc -o
obj/extensions/renderer/renderer/module_system.o
../../extensions/renderer/module_system.cc:495:5: error: ignoring return value
of function declared with 'warn_unused_result' attribute
[-Werror,-Wunused-result]
    object->Delete(context, property);

I think this is related to issue, https://codereview.chromium.org/2144093002/ in
chromium. These two tickets seem now to be dependant upon each other.
Could you advice my on how these situations are usually solved (if what I'm
saying above is correct that is, still very new to both code bases and builds)?

Thanks

[danbev, 2017-03-18 15:58:50]

Rebased with upstream/master. 

I suspect the chromium tests will fail again (the v8 quickchecks pass) as I'm
not sure how to fix that issue. Please see my previous comment about this.

[jochen (gone - plz use gerrit), 2017-03-20 07:13:52]

once the chromium side has landed, this CL should be fine

[jochen (gone - plz use gerrit), 2017-03-22 09:56:50]

the chromium side has landed, so let's give this a go

[jochen (gone - plz use gerrit), 2017-03-22 09:56:55]

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

[commit-bot: I haz the power, 2017-03-22 09:56:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 10:22:28]

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

[commit-bot: I haz the power, 2017-03-22 10:22:29]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2017-03-22 10:23:08]

The CQ bit was checked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2017-03-22 10:23:09]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
ahaas@chromium.org
Link to the patchset: https://codereview.chromium.org/2135973002/#ps140001
(title: "Using CHECK for Maybe<bool> results")

[commit-bot: I haz the power, 2017-03-22 10:23:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 10:24:29]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1490178188726010,
"parent_rev": "71fe3dd8d2faade9b51f079775252ce0a7652410", "commit_rev":
"4e92e124858c56bd0c99533395c9c2333d954e5f"}

[commit-bot: I haz the power, 2017-03-22 10:24:34]

Description was changed from

==========
Adding V8_WARN_UNUSED_RESULT for specified TODOs

Currently there are a number of comment in src/v8.h which look like
this: TODO(dcarney): mark V8_WARN_UNUSED_RESULT.

This commit attempts to remove these comments and add the
V8_WARN_UNUSED_RESULT macro to the methods in question.

BUG=
==========

to

==========
Adding V8_WARN_UNUSED_RESULT for specified TODOs

Currently there are a number of comment in src/v8.h which look like
this: TODO(dcarney): mark V8_WARN_UNUSED_RESULT.

This commit attempts to remove these comments and add the
V8_WARN_UNUSED_RESULT macro to the methods in question.

BUG=

Review-Url: https://codereview.chromium.org/2135973002
Cr-Commit-Position: refs/heads/master@{#44010}
Committed:
https://chromium.googlesource.com/v8/v8/+/4e92e124858c56bd0c99533395c9c2333d9...
==========

[commit-bot: I haz the power, 2017-03-22 10:24:36]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/v8/v8/+/4e92e124858c56bd0c99533395c9c2333d9...