Bug 28410 – Add 'icu::' qualifier to ICU C++ names

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jungshik Shin]

Chromium is going to upgrade its ICU to ICU 4.2.1. In the process, it's found that there's a name collision between Chromium and ICU because both has StringPiece in the global namespace. To prevent this, Chromium decided to build its ICU with U_USING_ICU_NAMESPACE set to 0 (the default is 1 and ICU dumps all the names to the global namespace). 

With that change, all the references to ICU's C++ names must be qualified with 'icu::'.  Fortunately, there are very few of them (most references to ICU apis are C APIs rather than C++ APIs). Chromium on Linux and Mac build just fine without any change. Chromium on Windows needs a minor change in a single file. 

However, there's a possibility that new references to ICU C++ names will be introduced to Webkit in the future. I'll send an email to webkit-dev about the issue (hoping that nobody will be against having to use 'icu::').

[Jungshik Shin]

Created attachment 34994 [details]
patch to add 'icu::'

[Eric Seidel (no email)]

I assume this is not up for review since it's not marked r=?.  It will need a ChangeLog before it can be reviewed.

[Eric Seidel (no email)]

I think there could be problems with this type of policy change, if this is not the default ICU compile options.  Mac WebKit is likely going to use the Mac OS X system installed ICU for the forseeable future.

[Jungshik Shin]

(In reply to comment #3)
> I think there could be problems with this type of policy change, if this is not
> the default ICU compile options.  Mac WebKit is likely going to use the Mac OS
> X system installed ICU for the forseeable future.

However, it can't hurt because what setting U_USING_ICU_NAMESPACE does is putting "using namespace icu;" for compilers that support C++ namespace (in the old days of ICU, I heard not many compilers support namespace). 

So, referring to ICU C++ names as 'icu::Foo' or 'Foo' does not make any difference when 'using namespace icu:' is in effect.  See my Chromium CL 
( http://codereview.chromium.org/171012/show )
that adds an explicit 'icu::' qualifier with U_USING_ICU_NAMESPACE still set to 1 (the CL was submitted yesterday and everything went fine :-) Somehow, our review tool didn't notice that it had been submitted :-) ).

[Jungshik Shin]

Created attachment 35045 [details]
patch with ChangeLog

I forgot to include ChangeLog in the previous patch as pointed out by Eric. This is ready for review.

[Eric Seidel (no email)]

Comment on attachment 35045 [details]
patch with ChangeLog

Ok, sounds fine.  I wonder if we shouldn't change the rest of our ICU uses to use icu:: if namespace icu { } is always defined and used by ICU these days.

[Jungshik Shin]

(In reply to comment #6)
> (From update of attachment 35045 [details])
> Ok, sounds fine.  I wonder if we shouldn't change the rest of our ICU uses to
> use icu:: if namespace icu { } is always defined and used by ICU these days.

I think that's a good idea as long as all the ports that use ICU also use compilers that support namespace, which is very likely in 2009 :-). 'namespace icu { }' is defined and used by ICU for compilers that support namespace. (actually, icu:: is an 'alias' to icu_M_N:: for icu M.N, but callers should stick to icu::). 

BTW, there are very few ICU C++ APIs in use in Webkit (actually, I haven't found one yet in non-Chromium part of WebKit). They all use C APIs which are not affected by this.

[Eric Seidel (no email)]

Comment on attachment 35045 [details]
patch with ChangeLog

Rejecting patch 35045 from commit-queue.  This patch will require manual commit.

Failed to run "['git', 'svn', 'rebase']"  exit_code: 1  cwd: None

[Eric Seidel (no email)]

Comment on attachment 35045 [details]
patch with ChangeLog

Sorry, we hit https://bugs.webkit.org/show_bug.cgi?id=28436

[Eric Seidel (no email)]

Comment on attachment 35045 [details]
patch with ChangeLog

Rejecting patch 35045 from commit-queue.  This patch will require manual commit.

Failed to run "['git', 'svn', 'rebase']"  exit_code: 1  cwd: None

[Eric Seidel (no email)]

Parsing ChangeLog: WebCore/ChangeLog
Updating working directory
error: patch failed: WebCore/ChangeLog:1
error: WebCore/ChangeLog: patch does not apply
rebase refs/remotes/trunk: command returned error: 1

bugzilla-tool is not yet smart enough to know hot to run resolve-ChangeLogs for you.

[Jungshik Shin]

I've just landed manually ( http://trac.webkit.org/changeset/47506 ).

[Jungshik Shin]

Created attachment 35136 [details]
patch pt2 (I missed a file)

Sorry that I missed this file earlier. With bug 28441 fixed, I built Webkit for Chromium on Windows and I'm sure there's no other reference to ICU C++ names.

[Eric Seidel (no email)]

Comment on attachment 35136 [details]
patch pt2 (I missed a file)

LGTM.

[Jungshik Shin]

Comment on attachment 35136 [details]
patch pt2 (I missed a file)

I'll land it manually because ChangeLog will be outdated again.