924019 - Do not assume that PRUnichar and UniChar are the same type

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[(no longer active)]

Attachment: 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.

Comment 1

[(no longer active)]

Attachment: Patch (v2)

Caught one more instance of this.

Comment 2

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 3

[(no longer active)]

(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!

Comment 4

[Steven Michaud [:smichaud] (Retired)]

> 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 :-)

Comment 5

[(no longer active)]

Attachment: Patch (v2)

OK, here's the new patch with the distinction between UniChar and unichar.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

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()];

Comment 7

[(no longer active)]

Haha, sure, will do.  Objective-C is very weird.  :-)

Comment 8

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ffbc9ebb0699

Comment 9

[Steven Michaud [:smichaud] (Retired)]

> 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 :-)

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ffbc9ebb0699