Skip to content

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Dec 6, 2017

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

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

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

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 danbev force-pushed the unused-var-node-contextify branch from 1e8a86b to 72e0878 Compare December 7, 2017 06:27
@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

@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
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
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
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
@MylesBorins
Copy link
Contributor

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

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.

8 participants