Closed Bug 956507 Opened 7 years ago Closed 7 years ago

Remove the PRUnichar typedef from Char16.h

Categories

(Core :: XPCOM, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(1 file)

Attached patch Patch (v1)Splinter Review
This is no longer needed.
Attachment #8355795 - Flags: review?(Pidgeot18)
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.
(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 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.
Attachment #8355795 - Flags: review?(Pidgeot18) → feedback+
We will have to continue the force-inclusion because of the poor MSVC.
(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 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!)
Attachment #8355795 - Flags: review?(Pidgeot18)
Attachment #8355795 - Flags: review?(Pidgeot18) → review+
Backed out, the patch has already bitrotten :(

https://hg.mozilla.org/integration/mozilla-inbound/rev/15f55912548a
https://hg.mozilla.org/mozilla-central/rev/3b06d1997d8c
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
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
(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!
Component: String → XPCOM
You need to log in before you can comment on or make changes to this bug.