Skip to content

src: clean up PER_ISOLATE_STRING_PROPERTIES#8207

Merged
bnoordhuis merged 2 commits into
nodejs:masterfrom
bnoordhuis:clean-up-per-isolate-strings
Aug 23, 2016
Merged

src: clean up PER_ISOLATE_STRING_PROPERTIES#8207
bnoordhuis merged 2 commits into
nodejs:masterfrom
bnoordhuis:clean-up-per-isolate-strings

Conversation

@bnoordhuis

@bnoordhuis bnoordhuis commented Aug 21, 2016

Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Aug 21, 2016
Comment thread src/node.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I find name confusing. Can we inline it or rename it to title_string?

@fhinkel

fhinkel commented Aug 21, 2016

Copy link
Copy Markdown
Member

LGTM.

@jasnell

jasnell commented Aug 21, 2016

Copy link
Copy Markdown
Member

LGTM

@bnoordhuis bnoordhuis force-pushed the clean-up-per-isolate-strings branch from c9e553c to 7e02c15 Compare August 23, 2016 11:50
@bnoordhuis

Copy link
Copy Markdown
Member Author

Renamed the variables. CI: https://ci.nodejs.org/job/node-test-pull-request/3796/

@jasnell

jasnell commented Aug 23, 2016

Copy link
Copy Markdown
Member

Still LGTM

Remove unused strings from the PER_ISOLATE_STRING_PROPERTIES list.

PR-URL: nodejs#8207
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Remove strings from the PER_ISOLATE_STRING_PROPERTIES list that are
only used once during initialization.  It's less expensive to simply
create them when needed than turn them into v8::Eternal instances.

PR-URL: nodejs#8207
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@bnoordhuis bnoordhuis force-pushed the clean-up-per-isolate-strings branch from 7e02c15 to b4ea3a0 Compare August 23, 2016 19:16
@bnoordhuis bnoordhuis closed this Aug 23, 2016
@bnoordhuis bnoordhuis deleted the clean-up-per-isolate-strings branch August 23, 2016 19:16
@bnoordhuis bnoordhuis merged commit b4ea3a0 into nodejs:master Aug 23, 2016
evanlucas pushed a commit that referenced this pull request Aug 24, 2016
Remove unused strings from the PER_ISOLATE_STRING_PROPERTIES list.

PR-URL: #8207
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request Aug 24, 2016
Remove strings from the PER_ISOLATE_STRING_PROPERTIES list that are
only used once during initialization.  It's less expensive to simply
create them when needed than turn them into v8::Eternal instances.

PR-URL: #8207
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins

Copy link
Copy Markdown
Contributor

@bnoordhuis should this be backported?

@bnoordhuis

Copy link
Copy Markdown
Member Author

If it applies and builds, sure. If it doesn't, it's okay to leave it out.

@MylesBorins

Copy link
Copy Markdown
Contributor

@bnoordhuis this does not land cleanly, leaving it out unless someone wants to backport

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++. lib / src Issues and PRs related to general changes in the lib or src directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants