Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

src: remove unused variable in node_contextify #17491

Closed
wants to merge 1 commit into from
Closed

src: remove unused variable in node_contextify #17491

wants to merge 1 commit into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Dec 6, 2017 •
edited
Loading

Currently the following warning is show when building:

../src/node_contextify.cc:642:10:
warning: unused variable 'display_errors' [-Wunused-variable]

    bool display_errors = maybe_display_errors.ToChecked();

This commit removes the unused variable which became unused in commit
aeddc36 ("src: remove tracking for exception arrow data").

Refs: #17394

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

src

@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 Dec 6, 2017
cjihrig
cjihrig reviewed Dec 6, 2017
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

Is this function call still needed?

@danbev
Copy link
Contributor Author

danbev commented Dec 6, 2017 •
edited
Loading

Is this function call still needed?

I'm really not sure if it is or not. I thought it would be safest to keep it and see what people think. @tniessen Do you by any chance know if this function call is needed?

@tniessen
Copy link
Member

tniessen commented Dec 6, 2017

@danbev That function should not have side effects, so it should be safe to remove the function call. I am not really sure how new vm.Script honors displayErrors despite not using that option 😕

@danbev
Copy link
Contributor Author

danbev commented Dec 6, 2017

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

@bnoordhuis
Copy link
Member

Maybe it's time we turn on -Werror for files in src/, at least for developer builds...

@danbev Can you note in the commit log that this was introduced in #17394 / aeddc36? Might be useful knowledge if it turns out it breaks something.

@danbev
src: remove unused variable in node_contextify
72e0878 
Currently the following warning is show when building:
../src/node_contextify.cc:642:10:
warning: unused variable 'display_errors' [-Wunused-variable]

    bool display_errors = maybe_display_errors.ToChecked();

This commit removes the unused variable which became unused in commit
aeddc36 ("src: remove tracking for
exception arrow data").

Refs: nodejs#17394
@danbev
Copy link
Contributor Author

danbev commented Dec 7, 2017

Can you note in the commit log that this was introduced in #17394 / aeddc36?

No problem, I've updated it now.

@danbev
Copy link
Contributor Author

danbev commented Dec 7, 2017

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

@danbev
Copy link
Contributor Author

danbev commented Dec 8, 2017

Landed in 51c6737

@danbev danbev closed this Dec 8, 2017
@danbev danbev deleted the unused-var-node-contextify branch December 8, 2017 10:27
danbev added a commit that referenced this pull request Dec 8, 2017
@danbev
src: remove unused variable in node_contextify
51c6737 
Currently the following warning is show when building:
../src/node_contextify.cc:642:10:
warning: unused variable 'display_errors' [-Wunused-variable]

    bool display_errors = maybe_display_errors.ToChecked();

This commit removes the unused variable which became unused in commit
aeddc36 ("src: remove tracking for
exception arrow data").

PR-URL: #17491
Refs: #17394
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins
Copy link
Contributor

This broke v9.x so setting don't land

Please feel free to change labels if it should land

@danbev
Copy link
Contributor Author

danbev commented Dec 12, 2017

@MylesBorins This commit depends on aeddc36 ("src: remove tracking for exception arrow data") PR: #17394, and should not break if that commit is applied first.

MylesBorins pushed a commit that referenced this pull request Dec 20, 2017
@danbev @MylesBorins
src: remove unused variable in node_contextify
0a0fb68 
Currently the following warning is show when building:
../src/node_contextify.cc:642:10:
warning: unused variable 'display_errors' [-Wunused-variable]

    bool display_errors = maybe_display_errors.ToChecked();

This commit removes the unused variable which became unused in commit
aeddc36 ("src: remove tracking for
exception arrow data").

PR-URL: #17491
Refs: #17394
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2017
@danbev @MylesBorins
src: remove unused variable in node_contextify
6bd0aff 
Currently the following warning is show when building:
../src/node_contextify.cc:642:10:
warning: unused variable 'display_errors' [-Wunused-variable]

    bool display_errors = maybe_display_errors.ToChecked();

This commit removes the unused variable which became unused in commit
aeddc36 ("src: remove tracking for
exception arrow data").

PR-URL: #17491
Refs: #17394
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
Merged
@MylesBorins
Copy link
Contributor

Most like won't make sense to land in LTS but setting watch in case we backport #17394

@MylesBorins MylesBorins mentioned this pull request Jan 22, 2018
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fhinkel fhinkel fhinkel approved these changes

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@danbev @tniessen @bnoordhuis @MylesBorins @fhinkel @addaleax @cjihrig @nodejs-github-bot