tools: add "icu::" namespace to ICU symbols by srl295 · Pull Request #18667 · nodejs/node

srl295 · 2018-02-09T00:16:19Z

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 · 2018-02-09T00:28:09Z

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

TimothyGu

The commit message needs to be revised. Otherwise LGTM.

bnoordhuis · 2018-02-09T13:48:41Z

src/inspector_io.cc

Long line, can you wrap this at 80 columns?

srl295 · 2018-02-09T16:19:16Z

@bnoordhuis wrapped.

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

TimothyGu · 2018-02-09T18:33:48Z

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

jasnell

nice. happy to see this.

srl295 · 2018-02-10T02:07:16Z

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

srl295 · 2018-03-28T14:07:06Z

@ilovezfs ^ label added as note to self…

ilovezfs · 2018-03-28T14:14:20Z

Thanks @srl295!

- 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>

- 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>

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

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

@kfarnung

[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>

… 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

@kfarnung

[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

[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 · 2018-06-14T14:37:07Z

What is the status for the v8.x backport?

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 self-assigned this Feb 9, 2018

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 force-pushed the icu-add-using branch from 6fa44c9 to c85423c Compare February 9, 2018 00:27

srl295 requested review from MylesBorins, TimothyGu, bnoordhuis and refack February 9, 2018 00:28

BridgeAR approved these changes Feb 9, 2018

View reviewed changes

TimothyGu reviewed Feb 9, 2018

View reviewed changes

bnoordhuis approved these changes Feb 9, 2018

View reviewed changes

src/inspector_io.cc Outdated

Copy link

Member

bnoordhuis Feb 9, 2018

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?

TimothyGu changed the title ~~tools: add "using namespace icu"~~ tools: add "icu::" namespace to ICU symbols Feb 9, 2018

jasnell approved these changes Feb 9, 2018

View reviewed changes

srl295 force-pushed the icu-add-using branch from 3ee37c1 to fd95720 Compare February 10, 2018 02:01

srl295 closed this Feb 10, 2018

srl295 force-pushed the icu-add-using branch from fd95720 to b8f47b2 Compare February 10, 2018 02:10

srl295 merged commit b8f47b2 into nodejs:master Feb 10, 2018

srl295 deleted the icu-add-using branch February 10, 2018 02:10

srl295 mentioned this pull request Feb 10, 2018

git-node-land: lease? nodejs/node-core-utils#172

Closed

srl295 mentioned this pull request Mar 26, 2018

deps: ICU 61.1 bump #19621

Merged

3 tasks

srl295 added the backport-requested-v8.x label Mar 28, 2018

kfarnung mentioned this pull request Apr 9, 2018

Set U_USING_ICU_NAMESPACE to 0 by default chakra-core/ChakraCore#4952

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

tools: add "icu::" namespace to ICU symbols #18667
srl295 merged 0 commit intonodejs:masterfrom
srl295:icu-add-using

srl295 commented Feb 9, 2018 •

edited

Loading

Uh oh!

srl295 commented Feb 9, 2018

Uh oh!

TimothyGu left a comment

Uh oh!

bnoordhuis Feb 9, 2018

Uh oh!

srl295 commented Feb 9, 2018

Uh oh!

TimothyGu commented Feb 9, 2018

Uh oh!

jasnell left a comment

Uh oh!

srl295 commented Feb 10, 2018

Uh oh!

srl295 commented Mar 28, 2018 •

edited

Loading

Uh oh!

ilovezfs commented Mar 28, 2018

Uh oh!

ArchangeGabriel commented Jun 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Uh oh!

Conversation

srl295 commented Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Affected core subsystem(s)

Uh oh!

srl295 commented Feb 9, 2018

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Feb 9, 2018

Choose a reason for hiding this comment

Uh oh!

srl295 commented Feb 9, 2018

Uh oh!

TimothyGu commented Feb 9, 2018

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

srl295 commented Feb 10, 2018

Uh oh!

srl295 commented Mar 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilovezfs commented Mar 28, 2018

Uh oh!

ArchangeGabriel commented Jun 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

srl295 commented Feb 9, 2018 •

edited

Loading

srl295 commented Mar 28, 2018 •

edited

Loading