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.

Sign up for GitHub

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

Jump to bottom

src: add missing #include <unicode/ustring.h> #11754

Merged
merged 0 commits into from
Mar 9, 2017
Merged

src: add missing #include <unicode/ustring.h> #11754

merged 0 commits into from
Mar 9, 2017

Conversation

srl295
Copy link
Member

@srl295 srl295 commented Mar 8, 2017 •
edited
Loading

  • we use functions that are in unicode/ustring.h
  • at present, this header is indirectly included, but this will change
  • no impact on past ICUs.

Fixes: #11753

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

i18n

@srl295 srl295 added the i18n-api Issues and PRs related to the i18n implementation. label Mar 8, 2017
@srl295 srl295 self-assigned this Mar 8, 2017
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. i18n-api Issues and PRs related to the i18n implementation. labels Mar 8, 2017
@srl295
Copy link
Member Author

srl295 commented Mar 8, 2017

this fixes the node side. v8 side will have to be done upstream

@srl295 srl295 requested review from jasnell and TimothyGu March 8, 2017 22:11
TimothyGu
TimothyGu reviewed Mar 9, 2017
View reviewed changes
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

Would be best if the commit message specifies which functions in ustring.h we are using right now.

@srl295
Copy link
Member Author

srl295 commented Mar 9, 2017

@TimothyGu good idea, will do

@srl295
Copy link
Member Author

srl295 commented Mar 9, 2017

@TimothyGu please re review

TimothyGu
TimothyGu approved these changes Mar 9, 2017
View reviewed changes
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

LGTM.

@srl295
Copy link
Member Author

srl295 commented Mar 9, 2017 •
edited
Loading

v8 issue will be fixed in https://codereview.chromium.org/2738503008 (work in progress)

It may be that other changes are needed as well, but this gets the immediate one.

@srl295 srl295 closed this Mar 9, 2017
@srl295 srl295 merged commit 5efb15e into nodejs:master Mar 9, 2017
@srl295 srl295 deleted the fix_include_ustring branch March 9, 2017 18:10
italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 13, 2017
@srl295 @italoacasas
src: add missing #include <unicode/ustring.h>
032becd 
* We use these functions that are declared in <unicode/ustring.h>

  u_strFromUTF8()
    u_strToUTF8()

* At present, <unicode/ustring.h> is indirectly included, but this will
likely change in future ICUs. Adding this header has been the right
thing to do for many years, so it is backwards compatible.

Fixes: nodejs#11753
PR-URL: nodejs#11754
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
@italoacasas italoacasas mentioned this pull request Mar 13, 2017
Merged
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
@srl295 @jungx098
src: add missing #include <unicode/ustring.h>
fddb23e 
* We use these functions that are declared in <unicode/ustring.h>

  u_strFromUTF8()
    u_strToUTF8()

* At present, <unicode/ustring.h> is indirectly included, but this will
likely change in future ICUs. Adding this header has been the right
thing to do for many years, so it is backwards compatible.

Fixes: nodejs#11753
PR-URL: nodejs#11754
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
@MylesBorins
Copy link
Contributor

@srl295 should this be backported to v6.x?

@MylesBorins
Copy link
Contributor

ping

@srl295
Copy link
Member Author

srl295 commented May 8, 2017

@MylesBorins If the patch applies, it doesn't hurt to backport it. It improves future proofing if someone were to use a later ICU with v6.x. Short answer, yes I think it should be.

@MylesBorins
Copy link
Contributor

@srl295 it doesn't land cleanly, please feel free to backport

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@jasnell jasnell jasnell approved these changes

Assignees

@srl295 srl295

Labels
c++ Issues and PRs that require attention from people who are familiar with C++. i18n-api Issues and PRs related to the i18n implementation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@srl295 @MylesBorins @jasnell @TimothyGu @cjihrig @nodejs-github-bot