Last Comment Bug 508905 - /Zc:wchar_t- is no longer required
: /Zc:wchar_t- is no longer required
Status: RESOLVED FIXED
fixed-in-bs
: dev-doc-needed
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: x86 Windows 7
: -- normal (vote)
: mozilla1.9.2b1
Assigned To: Jacek Caban
:
:
Mentors:
Depends on: 449292 493280 505727 505729 505734 507338 542091
Blocks:
  Show dependency treegraph
 
Reported: 2009-08-06 15:51 PDT by Masatoshi Kimura [:emk]
Modified: 2011-09-28 03:52 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
(Av1) patch in updater [Moved to bug 507338] (679 bytes, patch)
2009-08-07 07:18 PDT, Masatoshi Kimura [:emk]
robert.strong.bugs: review+
Details | Diff | Splinter Review
(Bv1) m-c patch (2.70 KB, patch)
2009-08-14 09:54 PDT, Masatoshi Kimura [:emk]
ted: review+
Details | Diff | Splinter Review
(Cv1-CC) comm-central patch (1.78 KB, patch)
2009-08-15 11:17 PDT, Masatoshi Kimura [:emk]
kairo: review+
Details | Diff | Splinter Review
(Cv1a-CC) added bug number per review comment (1.81 KB, patch)
2009-08-15 17:36 PDT, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Splinter Review
(Dv1-CC) followup-patch after m-c landing (1.72 KB, patch)
2009-08-15 17:36 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
(Cv2-CC) removed MOZILLA_1_9_1_BRANCH checks (1.70 KB, patch)
2009-10-25 09:58 PDT, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Splinter Review
(Dv2-CC) followup-patch after m-c landing (1.61 KB, patch)
2009-10-25 09:59 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
(Ev1) fix for wince (6.71 KB, patch)
2009-12-13 12:34 PST, Jacek Caban
no flags Details | Diff | Splinter Review
(Ev2) fix for wince (6.75 KB, patch)
2009-12-13 14:04 PST, Jacek Caban
no flags Details | Diff | Splinter Review
(Ev3) Final fix (10.10 KB, patch)
2009-12-16 07:38 PST, Jacek Caban
no flags Details | Diff | Splinter Review
(Ev4) fixed wchar_t/WCHAR [Checked in: Comment 57] (10.13 KB, patch)
2010-01-14 10:09 PST, Jacek Caban
doug.turner: review+
Details | Diff | Splinter Review
(Bv2) updated m-c patch (2.21 KB, patch)
2010-01-29 07:10 PST, Jacek Caban
no flags Details | Diff | Splinter Review
(Fv1) cairo-dwrite fix (727 bytes, patch)
2010-06-14 03:42 PDT, Jacek Caban
jmuizelaar: review-
Details | Diff | Splinter Review
(Fv2) cairo-dwrite fix x.1.1 [Checked in: Comment 57] (1.20 KB, patch)
2010-06-17 06:36 PDT, Jacek Caban
jmuizelaar: review+
Details | Diff | Splinter Review
(Gv1) Harfbuzz fix [Moved to bug 449292] (752 bytes, patch)
2010-06-18 02:49 PDT, neil@parkwaycc.co.uk
jd.bugzilla: review+
Details | Diff | Splinter Review
(Bv3) the patch with wchar_t enabled for _CC_SUITE=7 [Backed out: Comment 63] (2.43 KB, patch)
2010-06-18 11:51 PDT, Jacek Caban
ted: review+
mbeltzner: approval2.0-
Details | Diff | Splinter Review
(Hv1) jetpack fix (1.29 KB, patch)
2010-06-26 11:51 PDT, Jacek Caban
benjamin: review+
Details | Diff | Splinter Review
(Bv4) unbitrotted (2.92 KB, patch)
2011-04-30 14:29 PDT, Masatoshi Kimura [:emk]
ted: review+
Details | Diff | Splinter Review

Description Masatoshi Kimura [:emk] 2009-08-06 15:51:02 PDT
Per bug 324842, it was added due to "This conflicts with various SDK headers that still want to define WCHAR". Is it still true with Vista SDK or later? I don't see any SDK errors without /Zc:wchar_t-.
I'm tired to see gazillions of MinGW build errors related to wchar_t.
Comment 1 Masatoshi Kimura [:emk] 2009-08-07 07:18:16 PDT
Created attachment 393180 [details] [diff] [review]
(Av1) patch in updater
[Moved to bug 507338]
Comment 2 Masatoshi Kimura [:emk] 2009-08-07 23:25:57 PDT
I could build fine even with 2003SP1 SDK (and all patches of dependent bugs).
So we have no reason to insist on using /Zc:wchar_t- anymore.
Comment 3 Masatoshi Kimura [:emk] 2009-08-07 23:54:31 PDT
Comment on attachment 393180 [details] [diff] [review]
(Av1) patch in updater
[Moved to bug 507338]

This will fix inconsistency with archivereader.h and readstrings.h
Comment 4 Robert Strong [:rstrong] (use needinfo to contact me) 2009-08-09 21:31:08 PDT
Comment on attachment 393180 [details] [diff] [review]
(Av1) patch in updater
[Moved to bug 507338]

Looks good and thanks
Comment 5 Dão Gottwald [:dao] 2009-08-10 01:26:24 PDT
http://hg.mozilla.org/mozilla-central/rev/cabad98babdb
Comment 6 Jacek Caban 2009-08-10 04:41:22 PDT
It's not fixed yet. The committed patch fixed only a bug that this bug depends on.
Comment 7 Masatoshi Kimura [:emk] 2009-08-10 06:48:00 PDT
Yes, I should have say that a follow-up patch will come. Reopening.
Comment 8 Dão Gottwald [:dao] 2009-08-10 10:59:40 PDT
Comment on attachment 393180 [details] [diff] [review]
(Av1) patch in updater
[Moved to bug 507338]

Backed out... This doesn't work on WinCE.

http://tinderbox.mozilla.org/showlog.cgi?log=Mobile/1249892930.1249893343.6278.gz
Comment 9 Jacek Caban 2009-08-10 11:43:36 PDT
It looks like windows.h (or winnt.h) needs to be included before progressui.h in progressui_null.cpp (or in progressui.h). I don't have setup to test it.
Comment 10 Robert Strong [:rstrong] (use needinfo to contact me) 2009-08-10 12:30:23 PDT
I'm working on getting the progress ui working on WinCE / WinMo and could take care of this over in bug 507338. Sorry I didn't catch this error... turns out my work on bug 507338 makes it so this patch compiles fine at least with Firefox WinCE.
Comment 11 Robert Strong [:rstrong] (use needinfo to contact me) 2009-08-14 01:03:12 PDT
bug 507338 landed on trunk and should land on the 1.9.2 branch in a couple of days
Comment 12 Masatoshi Kimura [:emk] 2009-08-14 09:54:47 PDT
Created attachment 394524 [details] [diff] [review]
(Bv1) m-c patch

Although this patch works with mozilla-central alone, it will break c-c + m-c build.
We will need a temporary configure option which controls /Zc:wchar_t- flag.
Comment 13 Masatoshi Kimura [:emk] 2009-08-15 11:17:05 PDT
Created attachment 394664 [details] [diff] [review]
(Cv1-CC) comm-central patch

This is a temporary patch to avoid c-c + m-c build being broken. The else clause will be eventually removed.
Comment 14 Robert Kaiser 2009-08-15 11:53:12 PDT
Comment on attachment 394664 [details] [diff] [review]
(Cv1-CC) comm-central patch

Oh, I see, you force this for now, then remove it from m-c then remove the else clause and make bot consistent again.

r=me on the condition that you will post a followup patch when the main one has landed on m-c to remove the else clause. Nit: please add a "(see bug 508905)" or so to the comment lines so that it's clear what this does.
Comment 15 Masatoshi Kimura [:emk] 2009-08-15 17:36:02 PDT
Created attachment 394682 [details] [diff] [review]
(Cv1a-CC) added bug number per review comment

Thank you!
Comment 16 Masatoshi Kimura [:emk] 2009-08-15 17:36:45 PDT
Created attachment 394683 [details] [diff] [review]
(Dv1-CC) followup-patch after m-c landing
Comment 17 Masatoshi Kimura [:emk] 2009-08-15 17:40:40 PDT
Comment on attachment 394524 [details] [diff] [review]
(Bv1) m-c patch

This patch needs to be landed after attachment 394682 [details] [diff] [review] and before 394683.
Comment 18 Masatoshi Kimura [:emk] 2009-08-17 13:30:41 PDT
Comment on attachment 394524 [details] [diff] [review]
(Bv1) m-c patch

pending review until a build error caused by 493280 is fixed.
Comment 19 Masatoshi Kimura [:emk] 2009-08-19 05:58:39 PDT
Comment on attachment 394524 [details] [diff] [review]
(Bv1) m-c patch

The fix was pushed. Requesting review again.
Comment 20 Serge Gautherie (:sgautherie) 2009-10-23 11:47:18 PDT
Comment on attachment 394682 [details] [diff] [review]
(Cv1a-CC) added bug number per review comment

Could you remove the MOZILLA_1_9_1_BRANCH checks now that c-c has branched? Thanks.
Comment 21 Masatoshi Kimura [:emk] 2009-10-23 11:55:59 PDT
Unfortunately, trunk will be broken again if the patch is applied :(
Comment 22 Masatoshi Kimura [:emk] 2009-10-25 09:58:22 PDT
Created attachment 408276 [details] [diff] [review]
(Cv2-CC) removed MOZILLA_1_9_1_BRANCH checks

Someone seems to have fixed offending codes.
Carrying forward r+.
Comment 23 Masatoshi Kimura [:emk] 2009-10-25 09:59:37 PDT
Created attachment 408277 [details] [diff] [review]
(Dv2-CC) followup-patch after m-c landing
Comment 24 Serge Gautherie (:sgautherie) 2009-10-25 10:23:12 PDT
Could you summarize which patch will land where/when/why? Thanks.
Comment 25 Masatoshi Kimura [:emk] 2009-10-25 19:05:30 PDT
(In reply to comment #24)
1. Land attachment #408276 [details] [diff] [review] on comm-central first. This patch will force /Zc:wchar_t- to subdir.
2. If tinderbox doesn't burn, land attachment #394524 [details] [diff] [review] on mozilla-central. This patch removes /Zc:wchar_t- from m-c. c-c will be built with /Zc:wchar_t- at this point.
3. If tinderbox doesn't burn, land attachment #408277 [details] [diff] [review] on comm-central. /Zc:wchar_t- will be removed from c-c also.
The order is important to avoid c-c build being broken.
Comment 26 Serge Gautherie (:sgautherie) 2009-10-25 20:16:44 PDT
(In reply to comment #25)

> (In reply to comment #24)
> 1. Land attachment #408276 [details] [diff] [review] on comm-central first. This patch will force
> /Zc:wchar_t- to subdir.

If you're unsure that this patch will pass, then it should probably be submitted to c-c Try server first, should it not?

> 2. If tinderbox doesn't burn, land attachment #394524 [details] [diff] [review] on mozilla-central. This
> patch removes /Zc:wchar_t- from m-c. c-c will be built with /Zc:wchar_t- at
> this point.

Was that tried at least locally, for m-c alone then c-c + m-c?

> 3. If tinderbox doesn't burn, land attachment #408277 [details] [diff] [review] on comm-central.
> /Zc:wchar_t- will be removed from c-c also.
> The order is important to avoid c-c build being broken.

That said,
isn't it possible to do all this the other way round: just removing existing code(s)?
(I guess not, but why?)
Comment 27 Masatoshi Kimura [:emk] 2009-10-28 05:31:16 PDT
(In reply to comment #26)
> If you're unsure that this patch will pass,
I believe the patch will pass. I just worried about unexpected contingency.

> then it should probably be
> submitted to c-c Try server first, should it not?
How to get a try server access? (Sorry for the dumb question.)

> Was that tried at least locally, for m-c alone then c-c + m-c?
Yes. I've tested all of combinations locally.

> That said,
> isn't it possible to do all this the other way round: just removing existing
> code(s)?
I'm concerned about timing issue. Is it possible to push the patches to c-c and m-c at the same time?
Comment 28 Serge Gautherie (:sgautherie) 2009-10-28 10:23:31 PDT
(In reply to comment #27)
> (In reply to comment #26)

> > submitted to c-c Try server first, should it not?
> How to get a try server access? (Sorry for the dumb question.)

https://wiki.mozilla.org/Thunderbird/Infrastructure/TryServer#Gaining_Access

> > isn't it possible to do all this the other way round: just removing existing
> > code(s)?
> I'm concerned about timing issue. Is it possible to push the patches to c-c and
> m-c at the same time?

Yes, each patch could be pushed to its respective repo one (right) after the other.

Unless there is some kind of need for clobber or depend builds that would brake and not recover otherwise, the two target patches should just be pushed directly, at the "same" time if you say so ;-)
(A transient red/orange (on c-c) would be no problem.)
Comment 30 Jacek Caban 2009-12-13 12:34:43 PST
Created attachment 417377 [details] [diff] [review]
(Ev1) fix for wince

The attached patch should fix the problem, but I don't have compile environment to test it. (Also there are some duplications between environment.h and mozce_shunt.h and GetEnvironmentVariableW returns char instead of DWORD, but these are not related problems).
Comment 32 Jacek Caban 2009-12-13 14:03:49 PST
Yeah :) I guess I might try setting up an environment if there will be more problems. I will attach another try that should fix this issue using wchar_t instead of WCHAR in header files as I've found that mozce_shunt.h is included by command line options, so WCHAR is not yet defined then.
Comment 33 Jacek Caban 2009-12-13 14:04:35 PST
Created attachment 417387 [details] [diff] [review]
(Ev2) fix for wince

Another try
Comment 34 Phil Ringnalda (:philor) 2009-12-13 14:34:00 PST
Another 100+ errors - http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1260742122.1260742781.12598.gz&fulltext=1
Comment 35 Jacek Caban 2009-12-16 07:38:31 PST
Created attachment 417929 [details] [diff] [review]
(Ev3) Final fix

I've setup an environment for WinCE building and the attached patch works for me.
Comment 36 Doug Turner (:dougt) 2009-12-16 09:25:34 PST
Comment on attachment 417929 [details] [diff] [review]
(Ev3) Final fix

at quick glance, this is a "unsigned short" -> WCHAR change.  Why is it important?
Comment 37 Jacek Caban 2009-12-16 10:20:40 PST
wchar_t is usually a builtin compiler type, so it's not the same as unsigned short and explicit casts are needed when we mix wchar_t and unsigned short. It's not true in MSC if you pass /Zc:wchar_t- option and that's what's currently done. It causes patches without explicit casts to be accepted. MinGW uses builtin type and thus compilation on mingw break often. We want to get rid of /Zc:wchar_t- so that both compilers will tread wchar_t the same way (another argument is a technical correctness).
Comment 38 Jacek Caban 2010-01-11 13:40:24 PST
Doug, do you need more explanation? Could you please review the patch?
Comment 39 Doug Turner (:dougt) 2010-01-11 13:53:53 PST
Comment on attachment 417929 [details] [diff] [review]
(Ev3) Final fix

why do we need to use both WCHAR and wchar_t?

I think the functions that we expose should match whatever msdn does.  For example:

_wgetcwd  should be 

wchar_t *_wgetcwd( 
   wchar_t *buffer,
   int maxlen 
);

that is, wchar_t not WCHAR.  is there a problem doing it this way?
Comment 40 Jacek Caban 2010-01-14 10:09:00 PST
Created attachment 421656 [details] [diff] [review]
(Ev4) fixed wchar_t/WCHAR
[Checked in: Comment 57]

Thanks for review, I've changed it. We can't change wchar_t->WCHAR in headers of functions like GetEnvironmentVariable in headers, because they are used in context that doesn't have WCHAR (and LPWSTR/LPCWSTR) defined. I don't think it's something we should care about.

There are other things that differ from MSDN like char instead of BOOL (that is defined as int), but it's out of scope of the problem this patch is meant to fix.
Comment 41 Jacek Caban 2010-01-29 07:10:07 PST
Created attachment 424234 [details] [diff] [review]
(Bv2) updated m-c patch

Thanks for review. I'm attaching updated m-c patch, that may be landed after "fixed wchar_t/WCHAR" (preferable with try server check first).
Comment 42 Justin Wood (:Callek) 2010-04-27 19:36:53 PDT
With both these patches applied; I pushed a try build, and it failed on windows:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1272347054.1272350184.647.gz&fulltext=1

e:/builds/moz2_slave/win32-hg/build/gfx/cairo/cairo/src/cairo-dwrite-font.cpp(280) : error C2664: 'DWriteFactory::FindSystemFontFamily' : cannot convert parameter 1 from 'uint16_t *' to 'const WCHAR *'
        Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or function-style cast

(among other warnings).

Once it is safe to push please add the keyword again.
Comment 43 Serge Gautherie (:sgautherie) 2010-04-28 04:24:25 PDT
(In reply to comment #42)
> With both these patches applied; I pushed a try build, and it failed on
> windows:

Interesting: how do you add a m-c patch to a MozillaTry build?
Comment 44 Serge Gautherie (:sgautherie) 2010-04-28 04:26:10 PDT
(In reply to comment #43)
> Interesting: how do you add a m-c patch to a MozillaTry build?

Never mind: I misread MozillaTry as ThunderbirdTry :-<
Comment 45 Jacek Caban 2010-06-14 03:42:06 PDT
Created attachment 451008 [details] [diff] [review]
(Fv1) cairo-dwrite fix

The attached patch fixes the cairo problem. This patch together with my fix from bug 449292 allows landing this bug (tested on try server).
Comment 46 Jeff Muizelaar [:jrmuizel] 2010-06-15 10:33:56 PDT
Comment on attachment 451008 [details] [diff] [review]
(Fv1) cairo-dwrite fix

>diff --git a/gfx/cairo/cairo/src/cairo-dwrite-font.cpp b/gfx/cairo/cairo/src/cairo-dwrite-font.cpp
>index 127ea85..0f12fbd 100644
>--- a/gfx/cairo/cairo/src/cairo-dwrite-font.cpp
>+++ b/gfx/cairo/cairo/src/cairo-dwrite-font.cpp
>@@ -277,7 +277,7 @@ _cairo_dwrite_font_face_create_for_toy (cairo_toy_font_face_t   *toy_face,
>     status = _cairo_utf8_to_utf16 (toy_face->family, -1,
> 				   &face_name, &face_name_len);
> 
>-    IDWriteFontFamily *family = DWriteFactory::FindSystemFontFamily(face_name);
>+    IDWriteFontFamily *family = DWriteFactory::FindSystemFontFamily(reinterpret_cast<wchar_t*>(face_name));
>     if (!family) {
> 	*font_face = (cairo_font_face_t*)&_cairo_font_face_nil;
> 	return CAIRO_STATUS_FONT_TYPE_MISMATCH;

Perhaps we can use MultiByteToWideChar instead of _cairo_utf8_to_utf16 so that we get the right type and don't need to cast?
Comment 47 Jacek Caban 2010-06-17 06:36:45 PDT
Created attachment 451923 [details] [diff] [review]
(Fv2) cairo-dwrite fix x.1.1
[Checked in: Comment 57]

Thanks for the review. The attached version uses MultiByteToWideChar (also fixes memory leak in error path).
Comment 48 Jacek Caban 2010-06-17 09:03:49 PDT
Patches from comment 40 and comment 47 can be checked in. After landing the patch from bug 449292 comment 146, the final patch from comment 41 can be landed. I'd appreciate quick checkin to avoid another breakages preventing landing the final patch.
Comment 49 neil@parkwaycc.co.uk 2010-06-18 02:49:25 PDT
Created attachment 452219 [details] [diff] [review]
(Gv1) Harfbuzz fix
[Moved to bug 449292]

hb_buffer_add_utf16 expects uint16_t* but aString is a PRUnichar*
Comment 50 neil@parkwaycc.co.uk 2010-06-18 02:52:20 PDT
Comment on attachment 424234 [details] [diff] [review]
(Bv2) updated m-c patch

>@@ -558,7 +558,6 @@ case "$target" in
>             _CC_SUITE=7
>         elif test "$_CC_MAJOR_VERSION" = "14"; then
>             _CC_SUITE=8
>-            CXXFLAGS="$CXXFLAGS -Zc:wchar_t-"
Would it be possible to add CXXFLAGS="$CXXFLAGS -Zc:wchar_t" to the _CC_SUITE=7 block? (-Zc:wchar_t- is the default for that version.)
Comment 51 Jacek Caban 2010-06-18 03:55:43 PDT
(In reply to comment #49)
> Created an attachment (id=452219) [details]
> Harfbuzz fix
> 
> hb_buffer_add_utf16 expects uint16_t* but aString is a PRUnichar*

This is what bug 449292 comment 146 is about.

(In reply to comment #50)
> (From update of attachment 424234 [details] [diff] [review])
> >@@ -558,7 +558,6 @@ case "$target" in
> >             _CC_SUITE=7
> >         elif test "$_CC_MAJOR_VERSION" = "14"; then
> >             _CC_SUITE=8
> >-            CXXFLAGS="$CXXFLAGS -Zc:wchar_t-"
> Would it be possible to add CXXFLAGS="$CXXFLAGS -Zc:wchar_t" to the _CC_SUITE=7
> block? (-Zc:wchar_t- is the default for that version.)

I think it should be possible, but I don't have one to test.
Comment 52 neil@parkwaycc.co.uk 2010-06-18 04:09:54 PDT
(In reply to comment #51)
> (In reply to comment #49)
> > Created an attachment (id=452219)
> > Harfbuzz fix
> > hb_buffer_add_utf16 expects uint16_t* but aString is a PRUnichar*
> This is what bug 449292 comment 146 is about.
Sorry for the duplication.

> (In reply to comment #50)
> > (From update of attachment 424234 [details] [diff] [review])
> > >@@ -558,7 +558,6 @@ case "$target" in
> > >             _CC_SUITE=7
> > >         elif test "$_CC_MAJOR_VERSION" = "14"; then
> > >             _CC_SUITE=8
> > >-            CXXFLAGS="$CXXFLAGS -Zc:wchar_t-"
> > Would it be possible to add CXXFLAGS="$CXXFLAGS -Zc:wchar_t" to the _CC_SUITE=7
> > block? (-Zc:wchar_t- is the default for that version.)
> I think it should be possible, but I don't have one to test.
How do you think I found the Harfbuzz compile error ;-)

(In reply to comment #0)
> Per bug 324842, it was added due to "This conflicts with various SDK headers
> that still want to define WCHAR". Is it still true with Vista SDK or later? I
> don't see any SDK errors without /Zc:wchar_t-.
I think VC7.1 was partly converted to wchar_t - WinNT.h uses it, but MAPIDefS.h and WabDefs.h still used WORD; the 2003 SDK uses wchar_t throughout.
Comment 53 Jacek Caban 2010-06-18 11:51:48 PDT
Created attachment 452314 [details] [diff] [review]
(Bv3) the patch with wchar_t enabled for _CC_SUITE=7
[Backed out: Comment 63]

> > > Would it be possible to add CXXFLAGS="$CXXFLAGS -Zc:wchar_t" to the _CC_SUITE=7
> > > block? (-Zc:wchar_t- is the default for that version.)
> > I think it should be possible, but I don't have one to test.
> How do you think I found the Harfbuzz compile error ;-)

Let's add it then :)
Comment 54 Masatoshi Kimura [:emk] 2010-06-18 15:24:28 PDT
Reassigning to an actual current contributer. Thank you, Jacek!
(In reply to comment #52)
> (In reply to comment #0)
> > Per bug 324842, it was added due to "This conflicts with various SDK headers
> > that still want to define WCHAR". Is it still true with Vista SDK or later? I
> > don't see any SDK errors without /Zc:wchar_t-.
> I think VC7.1 was partly converted to wchar_t - WinNT.h uses it, but MAPIDefS.h
> and WabDefs.h still used WORD; the 2003 SDK uses wchar_t throughout.
I only considered VC8 or later because /Zc:wchar_t- was the default for VC7.1.
I don't oppose adding a option for VC7.1 because it seems harmless.
Comment 55 Masatoshi Kimura [:emk] 2010-06-26 02:53:47 PDT
Note that this patch will conflict with a patch for bug 570365.
Comment 56 Jacek Caban 2010-06-26 11:51:21 PDT
Created attachment 454286 [details] [diff] [review]
(Hv1) jetpack fix

Jetpack added new PRUnichar->jschar problem. The attached patch fixes it.
Comment 57 Jacek Caban 2010-06-27 02:37:44 PDT
http://hg.mozilla.org/mozilla-central/rev/9ca328e60bdd
http://hg.mozilla.org/mozilla-central/rev/dceb0c65850a

The final configure.in patch is blocked by jetpack fix.
Comment 58 Benjamin Smedberg [:bsmedberg] 2010-07-16 13:40:06 PDT
Comment on attachment 454286 [details] [diff] [review]
(Hv1) jetpack fix

talk about ugly :-(
Comment 59 Jacek Caban 2010-07-17 01:38:18 PDT
(In reply to comment #58)
> Comment on attachment 454286 [details] [diff] [review]
> jetpack fix
> 
> talk about ugly :-(

Thanks for the review. There is bug 578340 in hope to make things less ugly...
Comment 60 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-08-04 12:08:06 PDT
Have you tested that sizeof(PRUnichar) is still 2 with this patch?

I think nsString.cpp should perhaps contain:

PR_STATIC_ASSERT(sizeof(PRUnichar) == 2)
PR_STATIC_ASSERT(sizeof(nsString::char_type) == 2)
PR_STATIC_ASSERT(sizeof(nsCString::char_type) == 1)

to double-check that we don't break those.
Comment 61 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-08-04 12:28:25 PDT
I'll deal with comment 60 in a different bug, though.
Comment 62 Jacek Caban 2010-08-07 08:48:33 PDT
Thanks. Pushed to m-c:

http://hg.mozilla.org/mozilla-central/rev/f10fc9e3be99
Comment 64 Mike Beltzner [:beltzner, not reading bugmail] 2011-02-23 10:16:58 PST
Comment on attachment 452314 [details] [diff] [review]
(Bv3) the patch with wchar_t enabled for _CC_SUITE=7
[Backed out: Comment 63]

(bookkeeping)
Comment 65 Masatoshi Kimura [:emk] 2011-04-30 14:29:31 PDT
Created attachment 529312 [details] [diff] [review]
(Bv4) unbitrotted

Let's try again.
Tryserver log:
http://tbpl.mozilla.org/?tree=Try&rev=53cdd1b87b28
Jetpack failures are known. Non-windows test failures are obviously unrelated.
Comment 66 Ted Mielczarek [:ted.mielczarek] 2011-05-06 06:23:37 PDT
Comment on attachment 529312 [details] [diff] [review]
(Bv4) unbitrotted

Review of attachment 529312 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 67 Jacek Caban 2011-05-12 03:07:33 PDT
Pushed to b-s:

http://hg.mozilla.org/projects/build-system/rev/f8849773f7f4
Comment 68 Ted Mielczarek [:ted.mielczarek] 2011-06-13 06:17:03 PDT
http://hg.mozilla.org/mozilla-central/rev/f8849773f7f4
Comment 69 neil@parkwaycc.co.uk 2011-06-21 03:06:19 PDT
Is it expected that my build will need a clobber after updating to this push? I had a few link errors due to mismatched types (wchar_t vs. unsigned short).
Comment 70 Jacek Caban 2011-06-23 06:52:44 PDT
Yes, it's quite likely, every file using wchar_t needs to be recompiled

Note You need to log in before you can comment on or make changes to this bug.