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: hide InitializeICUDirectory symbol #1815

Merged
merged 1 commit into from
Jun 2, 2015

Conversation

bnoordhuis
Copy link
Member

Exporting it seems like an oversight. It's not safe to call once
V8 is running so there doesn't seem to be a point in exporting it
to add-ons. Un-export it.

R=@srl295?

@cjihrig
Copy link
Contributor

cjihrig commented May 27, 2015

LGTM, but couldn't this be a breaking change?

@bnoordhuis
Copy link
Member Author

What would be affected by this? It's not usable from an add-on.

@cjihrig
Copy link
Contributor

cjihrig commented May 27, 2015

Just double checking.

@brendanashworth brendanashworth added the c++ Issues and PRs that require attention from people who are familiar with C++. label May 27, 2015
@bnoordhuis
Copy link
Member Author

@srl295 You want to comment? I'll merge it tomorrow otherwise.

Exporting it seems like an oversight.  It's not safe to call once
V8 is running so there doesn't seem to be a point in exporting it
to add-ons.  Un-export it.

PR-URL: nodejs#1815
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@bnoordhuis bnoordhuis closed this Jun 2, 2015
@bnoordhuis bnoordhuis deleted the hide-i18n-symbol branch June 2, 2015 19:45
@bnoordhuis bnoordhuis merged commit 8c71a92 into nodejs:master Jun 2, 2015
andrewdeandrade pushed a commit to andrewdeandrade/node that referenced this pull request Jun 3, 2015
Exporting it seems like an oversight.  It's not safe to call once
V8 is running so there doesn't seem to be a point in exporting it
to add-ons.  Un-export it.

PR-URL: nodejs/node#1815
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@rvagg rvagg mentioned this pull request Jun 11, 2015
@srl295
Copy link
Member

srl295 commented Jul 29, 2015

@bnoordhuis sorry— LGTM in arrears (I was away, just clearing the backlog). Thanks for the catch.

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.

4 participants