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: removing unused StringValue macro parameters #7905

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Jul 28, 2016

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

src

Description of change

Currently, there are a few places where macro functions passed to the
PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES and
PER_ISOLATE_STRING_PROPERTIES macros, don't use the StringValue parameter.

This commit removes the StringValue parameter where it is not used.

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jul 28, 2016
@bnoordhuis
Copy link
Member

The reason I left that in is so you don't have to go searching for the type of the second parameter.

@danbev
Copy link
Contributor Author

danbev commented Jul 28, 2016

The reason I left that in is so you don't have to go searching for the type of the second parameter.

Sorry if I'm misunderstanding you here. When I was reading the code I was looking for usage of StringValue, but could not find any (in the places covered by this PR I mean), despite the V macros having defined it as a parameter.

I'm happy to have this PR closed as I just wanted to raise this in case it was something that was overlooked.

@bnoordhuis
Copy link
Member

What I mean is that calling it 'ignored' means you have to go hunting for the PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES define if you want to know what the type of the second argument is, that's why I gave it a name that conveys that information (and also for consistency with other places where that macro is used.)

@danbev
Copy link
Contributor Author

danbev commented Jul 28, 2016

What I mean is that calling it 'ignored' means you have to go hunting for the PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES

Ah I see, thanks for clarifying. I'll change it back to StringValue.

@addaleax
Copy link
Member

Currently, there are a few places where macro functions passed to the
PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES and
PER_ISOLATE_STRING_PROPERTIES macros, don't use the StringValue parameter.

This commit removes the StringValue parameter where it is not used.
@danbev danbev force-pushed the removing-unused-StringValue-macro-parameter branch from f4e38f3 to c7b3871 Compare September 22, 2016 06:32
@danbev
Copy link
Contributor Author

danbev commented Sep 22, 2016

Sorry, I forgot about this PR. I've rebased it now.

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

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM, CI looks good

danbev added a commit to danbev/node that referenced this pull request Sep 23, 2016
Currently, there are a few places where macro functions passed to the
PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES and
PER_ISOLATE_STRING_PROPERTIES macros, don't use the StringValue parameter.

This commit removes the StringValue parameter where it is not used.

PR-URL: nodejs#7905
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@danbev
Copy link
Contributor Author

danbev commented Sep 23, 2016

Thanks for the reviews!

Landed in: 84eaa4a

@danbev danbev closed this Sep 23, 2016
@danbev danbev deleted the removing-unused-StringValue-macro-parameter branch September 26, 2016 04:00
jasnell pushed a commit that referenced this pull request Sep 29, 2016
Currently, there are a few places where macro functions passed to the
PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES and
PER_ISOLATE_STRING_PROPERTIES macros, don't use the StringValue parameter.

This commit removes the StringValue parameter where it is not used.

PR-URL: #7905
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@Fishrock123
Copy link
Contributor

resolves to an empty commit on v6

@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
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++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants