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

deps: turn on std::string for ICU #19624

Closed
wants to merge 0 commits into from
Closed

Conversation

srl295
Copy link
Member

@srl295 srl295 commented Mar 26, 2018

  • node and v8 did not call into std::string previously, so that access was shut off.
  • this fixes compilation for ICU 58.2 (backlevel) but may be expressed in other versions also.

Fixes: #19151

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@srl295 srl295 added the i18n-api Issues and PRs related to the i18n implementation. label Mar 26, 2018
@srl295 srl295 self-assigned this Mar 26, 2018
@srl295 srl295 requested a review from TimothyGu March 26, 2018 23:47
@nodejs-github-bot nodejs-github-bot added i18n-api Issues and PRs related to the i18n implementation. tools Issues and PRs related to the tools directory. labels Mar 26, 2018
Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

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

LGTM.

The commit message be "turn on" rather than "turn in", or maybe just "use".

@srl295 srl295 changed the title deps: turn in std::string for ICU deps: turn on std::string for ICU Mar 27, 2018
@srl295
Copy link
Member Author

srl295 commented Mar 27, 2018

@richardlau “turn in” is a typo. std::string was already used by v8, but the compilation options told ICU that std::string wasn't available.

srl295 added a commit that referenced this pull request Mar 28, 2018
- node and v8 did not call into std::string previously,
so that access was shut off.
- this fixes compilation for ICU 58.2 (backlevel) but may
be expressed in other versions also.

Fixes: #19151

PR-URL: #19624
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
@srl295 srl295 closed this Mar 28, 2018
@srl295
Copy link
Member Author

srl295 commented Mar 28, 2018

@richardlau oops, i had fixed the commit title but didn't do it properly before push (but after local test!)

@srl295 srl295 deleted the icu-fix-stdstring branch March 28, 2018 15:24
targos pushed a commit that referenced this pull request Apr 2, 2018
- node and v8 did not call into std::string previously,
so that access was shut off.
- this fixes compilation for ICU 58.2 (backlevel) but may
be expressed in other versions also.

Fixes: #19151

PR-URL: #19624
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
@targos targos mentioned this pull request Apr 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
i18n-api Issues and PRs related to the i18n implementation. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

deps: issues with v8 and prior ICUs
7 participants