Bug 28441 (icu42) - build error with ICU 4.2.x on Windows + Visual studio
Summary: build error with ICU 4.2.x on Windows + Visual studio
Status: RESOLVED FIXED
Alias: icu42
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Jungshik Shin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-18 17:15 PDT by Jungshik Shin
Modified: 2009-08-19 11:39 PDT (History)
5 users (show)

See Also:


Attachments
patch : prepend WTF_...ASCIIType_h with 'isXXX' / 'toXXX' (3.98 KB, patch)
2009-08-19 10:02 PDT, Jungshik Shin
darin: 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-18 17:15:01 PDT
When I tried to build Chromium with ICU 4.2.x on Windows, I came across exactly the same error as Peter encountered a couple of month ago ( http://paste.lisp.org/display/81781 ) except that I'm getting it in wtf/unicode/icu/CollatorICU.cpp. 

He fixed the problem by  using Vector<char> instead of std::string.  ( http://trac.webkit.org/changeset/44643 ). 

In this case, I'm afraid that's not an option because unicode/ucol.h pulls in unicode/unistr.h, which in turn pulls in unicode/std_string.h and <string>. On Windows, <string> pulls in <xlocale> with tolower, toupper, etc. They're all replaced with WTF_Please_use_ASCIICType_instead_of_ctype_see_comment_in_ASCIICType_h leading to compile errors (multiple declarations).

One work around the problem is to append "_foo" to WTF_Please_use_ASCIICType_instead_of_ctype_see_comment_in_ASCIICType_h for 'foo' #defined in DisallowCType.h

So, they'll look like:

#define tolower WTF_Please_use_ASCIICType_instead_of_ctype_see_comment_in_ASCIICType_h_tolower
#define toupper WTF_Please_use_ASCIICType_instead_of_ctype_see_comment_in_ASCIICType_h_toupper
Comment 1 Jungshik Shin 2009-08-18 17:19:36 PDT
Oops. I forgot to mention that ICU 4.2.x began to use std::string where it's supported. Chromium has  used ICU 3.8 and I'm about to upgrade it to ICU 4.2.1. I guess Safari will eventually move to ICU 4.2.x or later (Win Safari uses ICU 4.0, right?) even on OS X (it's almost certain that Mac OS 10.7 - after snow leopard - will have at least ICU 4.2.x). 

If the approach mentioned in the previous comment is acceptable, I'll make a patch.
Comment 2 Jungshik Shin 2009-08-19 10:02:24 PDT
Created attachment 35126 [details]
patch : prepend WTF_...ASCIIType_h with 'isXXX' / 'toXXX'
Comment 3 Eric Seidel (no email) 2009-08-19 10:40:38 PDT
I'm totally confused by this fix.  Why?
Comment 4 Jungshik Shin 2009-08-19 10:55:58 PDT
Let me try to clarify. 

CollatorICU.cpp (in Javascript/wtf/unicode/icu) includes ICU's i18n/unicode/ucol.h. Beginning with ICU 4.2, ucol.h pulls in  C++ standard <string> (not directly but via a chain of includes). 

<string> in turn pulls in <xlocale> (again indirectly). <xlocale> in Visual Studio has the following at line 1386

        _Elem __CLR_OR_THIS_CALL toupper(_Elem _Ch) const
                {       // convert element to upper case
                return (do_toupper(_Ch));
                }

At line 1375, it has 

        _Elem __CLR_OR_THIS_CALL tolower(_Elem _Ch) const
                {       // convert element to upper case
                return (do_toupper(_Ch));
                }


Because DisallowCType.h defines tolower and toupper (and other ctype macros/functions) to WTF_Please_use_ASCIICType_instead_of_ctype_see_comment_in_ASCIICType_h, the  above two end up being identical and leading to the errors in http://paste.lisp.org/display/81781  

toupper and tolower show up multiple times in 
<xlocale> (in "Microsoft Visual Studio 8\VC\include") so that there are multiple errors as shown in http://paste.lisp.org/display/81781
Comment 5 Jungshik Shin 2009-08-19 10:57:55 PDT
(In reply to comment #4)

> At line 1375, it has 
> 
>         _Elem __CLR_OR_THIS_CALL tolower(_Elem _Ch) const
>                 {       // convert element to upper case
>                 return (do_toupper(_Ch));
>                 }

Ooops. The above should read

        _Elem __CLR_OR_THIS_CALL tolower(_Elem _Ch) const
                {       // convert element to lower case
                return (do_tolower(_Ch));
                }
Comment 6 Darin Adler 2009-08-19 11:13:04 PDT
Comment on attachment 35126 [details]
patch : prepend WTF_...ASCIIType_h with 'isXXX' / 'toXXX'

This seems like an OK way to sidestep the build failure. Not a great solution, but something that will do no harm.

r=me
Comment 7 Jungshik Shin 2009-08-19 11:39:25 PDT
Thank you for r+. Just checked in ( http://trac.webkit.org/changeset/47516 )