956507 - Remove the PRUnichar typedef from Char16.h

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[(no longer active)]

Attachment: Patch (v1)

This is no longer needed.

Comment 1

[Masatoshi Kimura [:emk]]

Comment on attachment 8355795 [details] [diff] [review]
Patch (v1)

Review of attachment 8355795 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/Char16.h
@@ -168,5 @@
>  
>  #endif
>  
> -/* This is a temporary hack until bug 927728 is fixed. */
> -#define __PRUNICHAR__

I think we should leave |#define __PRUNICHAR__| because it will suppress typedef PRUnichar in prtypes.h. It will be very useful to prevent PRUnichar sneaking in again.

Comment 2

[Masatoshi Kimura [:emk]]

(In reply to Masatoshi Kimura [:emk] from comment #1)
> I think we should leave |#define __PRUNICHAR__| because it will suppress
> typedef PRUnichar in prtypes.h. It will be very useful to prevent PRUnichar
> sneaking in again.

Hm, it was harder than I thought first. We should rather remove the prtypes.h inclusion itself...

Comment 3

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8355795 [details] [diff] [review]
Patch (v1)

Review of attachment 8355795 [details] [diff] [review]:
-----------------------------------------------------------------

I have no qualms about this patch by itself. My understanding of the changes made to Char16.h, however, was that most of them were due to the force-inclusion of Char16.h globally. Therefore, I would like to see the force-inclusion of Char16.h removed and the other changes to Char16.h (making it work in C mode) undone.

Comment 4

[Masatoshi Kimura [:emk]]

We will have to continue the force-inclusion because of the poor MSVC.

Comment 5

[(no longer active)]

(In reply to Masatoshi Kimura [:emk] from comment #1)
> Comment on attachment 8355795 [details] [diff] [review]
> Patch (v1)
> 
> Review of attachment 8355795 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mfbt/Char16.h
> @@ -168,5 @@
> >  
> >  #endif
> >  
> > -/* This is a temporary hack until bug 927728 is fixed. */
> > -#define __PRUNICHAR__
> 
> I think we should leave |#define __PRUNICHAR__| because it will suppress
> typedef PRUnichar in prtypes.h. It will be very useful to prevent PRUnichar
> sneaking in again.

We can't do that, since that will make code which pulls in NSPR headers that use PRUnichar not compile (since PRUnichar will be defined by nothing then!).

Comment 6

[(no longer active)]

Comment on attachment 8355795 [details] [diff] [review]
Patch (v1)

(In reply to Joshua Cranmer [:jcranmer] from comment #3)
> Comment on attachment 8355795 [details] [diff] [review]
> Patch (v1)
> 
> Review of attachment 8355795 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have no qualms about this patch by itself. My understanding of the changes
> made to Char16.h, however, was that most of them were due to the
> force-inclusion of Char16.h globally. Therefore, I would like to see the
> force-inclusion of Char16.h removed and the other changes to Char16.h
> (making it work in C mode) undone.

Given our IRC conversations today and my promise to work on bug 957358 instead, please review this patch.

(Note that we should not let the perfect be the enemy of the good here, since any minute that we don't take this patch is a minute that somebody adds in code using PRUnichar which will happily compile anywhere in our tree.  Also note that even if I had the patch to stop force-including this header ready now, I'd still want to take this patch first!)

Comment 7

[(no longer active)]

Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/07a4682a75f2

Comment 8

[(no longer active)]

Backed out, the patch has already bitrotten :(

https://hg.mozilla.org/integration/mozilla-inbound/rev/15f55912548a

Comment 9

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b06d1997d8c

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3b06d1997d8c

Comment 11

[Jacek Caban]

I landed a trivial fix for wchar_t/char16_t mismatch causing an error on mingw:

https://hg.mozilla.org/integration/mozilla-inbound/rev/816e70bd19e7

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/816e70bd19e7

Comment 13

[(no longer active)]

(In reply to comment #11)
> I landed a trivial fix for wchar_t/char16_t mismatch causing an error on mingw:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/816e70bd19e7

Looks good!