Do not assume that PRUnichar and UniChar are the same type

RESOLVED FIXED in mozilla27

Status

()

Core
Widget: Cocoa
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

Trunk
mozilla27
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 814021 [details] [diff] [review]
Patch (v1)

Bug 895047 makes UniChar and PRUnichar be different compile-time types (unsigned short and char16_t respectively).  These types are ABI compatible, but we need to cast explicitly between them.
Attachment #814021 - Flags: review?(smichaud)
(Assignee)

Comment 1

5 years ago
Created attachment 814119 [details] [diff] [review]
Patch (v2)

Caught one more instance of this.
Attachment #814021 - Attachment is obsolete: true
Attachment #814021 - Flags: review?(smichaud)
Attachment #814119 - Flags: review?(smichaud)
Actually the Mac has both "unichar" and "UniChar", defined as follows:

typedef UInt16 UniChar; // In /usr/include/MacTypes.h
typedef unsigned short unichar; // In /System/Library/Frameworks/Foundation.framework/Headers/NSString.h

As best I can tell, Cocoa classes (like NSString) use "unichar".  But other, older APIs (like CFString) use "UniChar".  I'm quite sure they're ABI compatible, and likely to remain so.  But if you're going to the trouble to write this patch, it seems like you should treat them separately.
(Assignee)

Comment 3

5 years ago
(In reply to comment #2)
> Actually the Mac has both "unichar" and "UniChar", defined as follows:
> 
> typedef UInt16 UniChar; // In /usr/include/MacTypes.h
> typedef unsigned short unichar; // In
> /System/Library/Frameworks/Foundation.framework/Headers/NSString.h
> 
> As best I can tell, Cocoa classes (like NSString) use "unichar".  But other,
> older APIs (like CFString) use "UniChar".  I'm quite sure they're ABI
> compatible, and likely to remain so.  But if you're going to the trouble to
> write this patch, it seems like you should treat them separately.

UInt16 is unsigned short too, so UniChar and unichar actually map to the exact same type.  The reason that I wrote this patch is that over in bug 895047, I'm changing PRUnichar to be char16_t, which is its own built-in type that is not the same as unsigned short (but it's ABI compatible of course), and because of this, some parts of our code doesn't compile, which is what this patch fixes.  Otherwise, for things that map to the same underlying type (such as jschar and PRUnichar before and after bug 895047), we use the types interchangeably (same with NSPR PRIntN types and stdint types.)

That being said, if you still want me to go the extra mile to differentiate between unichar and UniChar here, I'll do that, but I just want to check with you first to see how much you care about that.  :-)

Thanks!
> if you still want me to go the extra mile to differentiate between
> unichar and UniChar here, I'll do that

Please do.  Otherwise this question will keep arising in the future,
as people try to figure out why we're passing a UniChar to Cocoa
methods :-)
(Assignee)

Comment 5

5 years ago
Created attachment 815406 [details] [diff] [review]
Patch (v2)

OK, here's the new patch with the distinction between UniChar and unichar.
Attachment #814119 - Attachment is obsolete: true
Attachment #814119 - Flags: review?(smichaud)
Attachment #815406 - Flags: review?(smichaud)
Comment on attachment 815406 [details] [diff] [review]
Patch (v2)

Looks fine to me.

But I have a style nit, which I forgot to mention earlier:

This

[NSString stringWithCharacters:reinterpret_cast<const unichar*>(blah)
          length:blah.Length()];

should be this (with the ':' characters lined up)

[NSString stringWithCharacters:reinterpret_cast<const unichar*>(blah)
                        length:blah.Length()];
Attachment #815406 - Flags: review?(smichaud) → review+
(Assignee)

Comment 7

5 years ago
Haha, sure, will do.  Objective-C is very weird.  :-)
> Objective-C is very weird.  :-)

It's a bit like a strong-flavored spice ... cilantro?  At first it tastes funny, but after a while you grow to like it :-)
https://hg.mozilla.org/mozilla-central/rev/ffbc9ebb0699
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.