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

tools: add "icu::" namespace to ICU symbols #18667

Merged
merged 0 commits into from
Feb 10, 2018
Merged

tools: add "icu::" namespace to ICU symbols #18667

merged 0 commits into from
Feb 10, 2018

Conversation

srl295
Copy link
Member

@srl295 srl295 commented Feb 9, 2018 •
edited
Loading

In ICU 61.x, icu4c will no longer put its declarations in the global namespace.
Everything will be in the "icu::" namespace (or icu_60:: in the linker).
Prepare for this.

Upstream: https://ssl.icu-project.org/trac/ticket/13460

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

tools

@srl295 srl295 added tools Issues and PRs related to the tools directory. i18n-api Issues and PRs related to the i18n implementation. labels Feb 9, 2018
@srl295 srl295 self-assigned this Feb 9, 2018
@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 Feb 9, 2018
@srl295
Copy link
Member Author

srl295 commented Feb 9, 2018

ICU 61.0 (in progress) works unmodified with this change.

TimothyGu
TimothyGu reviewed Feb 9, 2018
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.

The commit message needs to be revised. Otherwise LGTM.

bnoordhuis
bnoordhuis approved these changes Feb 9, 2018
View reviewed changes
src/inspector_io.cc Outdated
UnicodeString utf16 =
UnicodeString::fromUTF8(StringPiece(message.data(), message.length()));
icu::UnicodeString utf16 =
icu::UnicodeString::fromUTF8(icu::StringPiece(message.data(), message.length()));
Copy link
Member

Choose a reason for hiding this comment

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

Long line, can you wrap this at 80 columns?

@srl295
Copy link
Member Author

srl295 commented Feb 9, 2018

@bnoordhuis wrapped.

@TimothyGu yeah, scope changed as i was fixing it. Do you have a recommendation?

@TimothyGu
Copy link
Member

@srl295 Oh oops I meant the PR title. The commit message is already correct.

@TimothyGu TimothyGu changed the title tools: add "using namespace icu" tools: add "icu::" namespace to ICU symbols Feb 9, 2018
jasnell
jasnell approved these changes Feb 9, 2018
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

nice. happy to see this.

@srl295
Copy link
Member Author

srl295 commented Feb 10, 2018

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

@srl295 srl295 closed this Feb 10, 2018
@srl295 srl295 merged commit b8f47b2 into nodejs:master Feb 10, 2018
@srl295 srl295 deleted the icu-add-using branch February 10, 2018 02:10
@srl295 srl295 mentioned this pull request Feb 10, 2018
Closed
@srl295 srl295 mentioned this pull request Mar 26, 2018
Merged
3 tasks
@srl295
Copy link
Member Author

srl295 commented Mar 28, 2018 •
edited
Loading

@ilovezfs ^ label added as note to self… 

@ilovezfs
Copy link

Thanks @srl295!

srl295 added a commit to srl295/node that referenced this pull request Apr 3, 2018
@srl295
deps: ICU 61.1 bump
6421140 
- Update to released ICU 61.1, including:
  - CLDR 33 (many new languages and data improvements)
  - Many small API additions, improvements, and bug fixes
  - note: 'icu::' namespace is no longer used by default
   (Necessated nodejs#18667 )

PR-URL: nodejs#19621
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Apr 3, 2018
@srl295 @targos
deps: ICU 61.1 bump
58f61db 
- Update to released ICU 61.1, including:
  - CLDR 33 (many new languages and data improvements)
  - Many small API additions, improvements, and bug fixes
  - note: 'icu::' namespace is no longer used by default
   (Necessated #18667 )

PR-URL: #19621
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
kfarnung added a commit to kfarnung/ChakraCore that referenced this pull request Apr 9, 2018
@kfarnung
Set U_USING_ICU_NAMESPACE to 0 by default
a4ae437 
ICU61 updated the value of `U_USING_ICU_NAMESPACE` to 0 by default. In
order to maintain compatibility this change updates our ICU includes to
match and adds the `icu::` namespace explicitly when needed.

Refs: nodejs/node#18667
Refs: https://ssl.icu-project.org/trac/ticket/13460
@kfarnung kfarnung mentioned this pull request Apr 9, 2018
Merged
kfarnung added a commit to kfarnung/node-chakracore that referenced this pull request Apr 9, 2018
@kfarnung
Set U_USING_ICU_NAMESPACE to 0 by default
81141c4 
ICU61 updated the value of `U_USING_ICU_NAMESPACE` to 0 by default. In
order to maintain compatibility this change updates our ICU includes to
match and adds the `icu::` namespace explicitly when needed.

Refs: nodejs/node#18667
Refs: https://ssl.icu-project.org/trac/ticket/13460
chakrabot pushed a commit to chakra-core/ChakraCore that referenced this pull request Apr 9, 2018
@kfarnung
[MERGE #4952 @kfarnung] Set U_USING_ICU_NAMESPACE to 0 by default
7f1ab68 
Merge pull request #4952 from kfarnung:icu61

ICU61 updated the value of `U_USING_ICU_NAMESPACE` to 0 by default. In
order to maintain compatibility this change updates our ICU includes to
match and adds the `icu::` namespace explicitly when needed.

Refs: nodejs/node#18667
Refs: https://ssl.icu-project.org/trac/ticket/13460
chakrabot pushed a commit to nodejs/node-chakracore that referenced this pull request Apr 9, 2018
@kfarnung @chakrabot
deps: update ChakraCore to chakra-core/ChakraCore@7f1ab68f0a
29d44c2 
[MERGE #4952 @kfarnung] Set `U_USING_ICU_NAMESPACE` to 0 by default

Merge pull request #4952 from kfarnung:icu61

ICU61 updated the value of `U_USING_ICU_NAMESPACE` to 0 by default. In
order to maintain compatibility this change updates our ICU includes to
match and adds the `icu::` namespace explicitly when needed.

Refs: nodejs/node#18667
Refs: https://ssl.icu-project.org/trac/ticket/13460

Reviewed-By: chakrabot <chakrabot@users.noreply.github.com>
chakrabot pushed a commit to chakra-core/ChakraCore that referenced this pull request Apr 9, 2018
@kfarnung
[1.9>master] [MERGE #4952 @kfarnung] Set U_USING_ICU_NAMESPACE to 0…
9e617e7 
… by default

Merge pull request #4952 from kfarnung:icu61

ICU61 updated the value of `U_USING_ICU_NAMESPACE` to 0 by default. In
order to maintain compatibility this change updates our ICU includes to
match and adds the `icu::` namespace explicitly when needed.

Refs: nodejs/node#18667
Refs: https://ssl.icu-project.org/trac/ticket/13460
chakrabot pushed a commit to nodejs/node-chakracore that referenced this pull request Apr 9, 2018
@kfarnung @chakrabot
deps: update ChakraCore to chakra-core/ChakraCore@9e617e7b52
a024125 
[1.9>master] [MERGE #4952 @kfarnung] Set `U_USING_ICU_NAMESPACE` to 0 by default

Merge pull request #4952 from kfarnung:icu61

ICU61 updated the value of `U_USING_ICU_NAMESPACE` to 0 by default. In
order to maintain compatibility this change updates our ICU includes to
match and adds the `icu::` namespace explicitly when needed.

Refs: nodejs/node#18667
Refs: https://ssl.icu-project.org/trac/ticket/13460

Reviewed-By: chakrabot <chakrabot@users.noreply.github.com>
kfarnung added a commit to nodejs/node-chakracore that referenced this pull request Apr 10, 2018
@kfarnung
deps: update ChakraCore to chakra-core/ChakraCore@7f1ab68f0a
69a3d10 
[MERGE #4952 @kfarnung] Set `U_USING_ICU_NAMESPACE` to 0 by default

Merge pull request #4952 from kfarnung:icu61

ICU61 updated the value of `U_USING_ICU_NAMESPACE` to 0 by default. In
order to maintain compatibility this change updates our ICU includes to
match and adds the `icu::` namespace explicitly when needed.

Refs: nodejs/node#18667
Refs: https://ssl.icu-project.org/trac/ticket/13460

Reviewed-By: chakrabot <chakrabot@users.noreply.github.com>
@ArchangeGabriel
Copy link

What is the status for the v8.x backport?

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

@TimothyGu TimothyGu TimothyGu left review comments

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@refack refack Awaiting requested review from refack

@MylesBorins MylesBorins Awaiting requested review from MylesBorins

Assignees

@srl295 srl295

Labels
i18n-api Issues and PRs related to the i18n implementation. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@srl295 @TimothyGu @ilovezfs @ArchangeGabriel @bnoordhuis @jasnell @BridgeAR @nodejs-github-bot