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.

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

src: fix unused private field warning #28036

Closed
wants to merge 1 commit into from
Closed

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jun 3, 2019

This fixes the following warning (and subsequent warnings that occurred from fixing the original warning):

../src/inspector/runtime_agent.h:26:16: warning: private field 'env_' is not used [-Wunused-private-field]
  Environment* env_;
               ^
1 warning generated.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@ZYSzys ZYSzys added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol labels Jun 3, 2019
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 3, 2019

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

EDIT(cjihrig): CI was yellow.

@cjihrig cjihrig changed the title src: fix unused private field error src: fix unused private field warning Jun 3, 2019
Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

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

LGTM. Is the explicit related to the _env fix?

@cjihrig
Copy link
Contributor Author

cjihrig commented Jun 3, 2019

Is the explicit related to the _env fix?

Yes. After removing the unused constructor argument, you get the following warning: Zero-parameter constructors should not be marked explicit.

@danbev
Copy link
Contributor

danbev commented Jun 4, 2019

Could we fast track this?

If you agree please give a 👍 and if you disagree please remove the fast-track label.

@danbev danbev added the fast-track PRs that do not need to wait for 48 hours to land. label Jun 4, 2019
@danbev
Copy link
Contributor

danbev commented Jun 5, 2019

Landed in 2c76874.

@danbev danbev closed this Jun 5, 2019
danbev pushed a commit that referenced this pull request Jun 5, 2019
PR-URL: #28036
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@cjihrig cjihrig deleted the unused branch June 5, 2019 21:01
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
PR-URL: #28036
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR BridgeAR mentioned this pull request Jun 17, 2019
codebytere added a commit to electron/node that referenced this pull request Jul 3, 2019
Removed in nodejs/node#28036; this patch can
be removed when we update to v12.5.0
codebytere added a commit to electron/node that referenced this pull request Jul 3, 2019
Removed in nodejs/node#28036; this patch can
be removed when we update to v12.5.0
MarshallOfSound pushed a commit to electron/node that referenced this pull request Jul 4, 2019
Removed in nodejs/node#28036; this patch can
be removed when we update to v12.5.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. fast-track PRs that do not need to wait for 48 hours to land. inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants