Skip to content

Conversation

@bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Feb 6, 2017

Fix a warning that was introduced in commit 67af1ad ("src: refactor
CopyProperties to remove JS") from a few days ago. This particular
change was suggested by me, mea culpa.

Fixes the following warning:

../src/node_contextify.cc:151:13: warning: ignoring return
value of function declared with warn_unused_result attribute
[-Wunused-result]
            sandbox_obj->DefineProperty(context, key, *desc);

CI: https://ci.nodejs.org/job/node-test-pull-request/6233/

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem. labels Feb 6, 2017
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Fix a warning that was introduced in commit 67af1ad ("src: refactor
CopyProperties to remove JS") from a few days ago.  This particular
change was suggested by me, mea culpa.

Fixes the following warning:

    ../src/node_contextify.cc:151:13: warning: ignoring return
    value of function declared with warn_unused_result attribute
    [-Wunused-result]
                sandbox_obj->DefineProperty(context, key, *desc);

PR-URL: nodejs#11197
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@bnoordhuis bnoordhuis closed this Feb 9, 2017
@bnoordhuis bnoordhuis deleted the fix-build-warning branch February 9, 2017 13:55
@bnoordhuis bnoordhuis merged commit 215a649 into nodejs:master Feb 9, 2017
@bnoordhuis
Copy link
Member Author

Sorry, final commit is 6af0bfe. I overlooked Daniel's LGTM.

bnoordhuis added a commit that referenced this pull request Feb 9, 2017
Fix a warning that was introduced in commit 67af1ad ("src: refactor
CopyProperties to remove JS") from a few days ago.  This particular
change was suggested by me, mea culpa.

Fixes the following warning:

    ../src/node_contextify.cc:151:13: warning: ignoring return
    value of function declared with warn_unused_result attribute
    [-Wunused-result]
                sandbox_obj->DefineProperty(context, key, *desc);

PR-URL: #11197
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@italoacasas
Copy link

This is not landing clearly in v7.x-staging, any plan to backport?

@bnoordhuis
Copy link
Member Author

It's dependent on #11102 and that's tagged with dont-land labels. I'll add them here as well.

@italoacasas
Copy link

italoacasas commented Feb 9, 2017

I'm going to backport #11102 since that version of v8 is already on staging

I land #11102 already in staging

@bnoordhuis
Copy link
Member Author

@italoacasas Then this PR should merge cleanly now.

italoacasas pushed a commit that referenced this pull request Feb 13, 2017
Fix a warning that was introduced in commit 67af1ad ("src: refactor
CopyProperties to remove JS") from a few days ago.  This particular
change was suggested by me, mea culpa.

Fixes the following warning:

    ../src/node_contextify.cc:151:13: warning: ignoring return
    value of function declared with warn_unused_result attribute
    [-Wunused-result]
                sandbox_obj->DefineProperty(context, key, *desc);

PR-URL: #11197
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 14, 2017
Fix a warning that was introduced in commit 67af1ad ("src: refactor
CopyProperties to remove JS") from a few days ago.  This particular
change was suggested by me, mea culpa.

Fixes the following warning:

    ../src/node_contextify.cc:151:13: warning: ignoring return
    value of function declared with warn_unused_result attribute
    [-Wunused-result]
                sandbox_obj->DefineProperty(context, key, *desc);

PR-URL: nodejs#11197
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
krydos pushed a commit to krydos/node that referenced this pull request Feb 25, 2017
Fix a warning that was introduced in commit 67af1ad ("src: refactor
CopyProperties to remove JS") from a few days ago.  This particular
change was suggested by me, mea culpa.

Fixes the following warning:

    ../src/node_contextify.cc:151:13: warning: ignoring return
    value of function declared with warn_unused_result attribute
    [-Wunused-result]
                sandbox_obj->DefineProperty(context, key, *desc);

PR-URL: nodejs#11197
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants