Bug 28410 (icuns) - Add 'icu::' qualifier to ICU C++ names
Summary: Add 'icu::' qualifier to ICU C++ names
Status: RESOLVED FIXED
Alias: icuns
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Jungshik Shin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-17 15:24 PDT by Jungshik Shin
Modified: 2009-08-19 14:26 PDT (History)
2 users (show)

See Also:


Attachments
patch to add 'icu::' (1.66 KB, patch)
2009-08-17 15:32 PDT, Jungshik Shin
no flags Details | Formatted Diff | Diff
patch with ChangeLog (2.29 KB, patch)
2009-08-18 09:19 PDT, Jungshik Shin
eric: review+
eric: commit-queue-
Details | Formatted Diff | Diff
patch pt2 (I missed a file) (1.71 KB, patch)
2009-08-19 13:30 PDT, Jungshik Shin
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jungshik Shin 2009-08-17 15:24:19 PDT
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::').
Comment 1 Jungshik Shin 2009-08-17 15:32:05 PDT
Created attachment 34994 [details]
patch to add 'icu::'
Comment 2 Eric Seidel (no email) 2009-08-17 15:34:32 PDT
I assume this is not up for review since it's not marked r=?.  It will need a ChangeLog before it can be reviewed.
Comment 3 Eric Seidel (no email) 2009-08-17 15:36:14 PDT
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.
Comment 4 Jungshik Shin 2009-08-18 09:14:06 PDT
(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 :-) ).
Comment 5 Jungshik Shin 2009-08-18 09:19:09 PDT
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.
Comment 6 Eric Seidel (no email) 2009-08-18 09:22:59 PDT
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.
Comment 7 Jungshik Shin 2009-08-18 10:18:16 PDT
(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.
Comment 8 Eric Seidel (no email) 2009-08-18 14:12:10 PDT
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
Comment 9 Eric Seidel (no email) 2009-08-18 14:24:12 PDT
Comment on attachment 35045 [details]
patch with ChangeLog

Sorry, we hit https://bugs.webkit.org/show_bug.cgi?id=28436
Comment 10 Eric Seidel (no email) 2009-08-18 15:08:32 PDT
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
Comment 11 Eric Seidel (no email) 2009-08-18 15:10:39 PDT
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.
Comment 12 Jungshik Shin 2009-08-19 09:39:51 PDT
I've just landed manually ( http://trac.webkit.org/changeset/47506 ).
Comment 13 Jungshik Shin 2009-08-19 13:30:58 PDT
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.
Comment 14 Eric Seidel (no email) 2009-08-19 13:51:32 PDT
Comment on attachment 35136 [details]
patch pt2 (I missed a file)

LGTM.
Comment 15 Jungshik Shin 2009-08-19 14:26:32 PDT
Comment on attachment 35136 [details]
patch pt2 (I missed a file)

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