900806 - Make nsString use char16_t internally

Status: RESOLVED DUPLICATE of bug 895047

(Core :: XPCOM, defect)

Description

[Joshua Cranmer [:jcranmer]]

Attachment: string-uses-char16

All of our platforms should support char16_t via mozilla/Char16.h at this point. This turns out to be a fairly non-invasive way to forcibly use it where we get a lot of benefits (debuggers will actually be able to print out the contents !!!)

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Could you add static_asserts that char16_t is unsigned and has sizeof() == 2?  (I think we have those for PRUnichar somewhere.)

Is this valid in terms of the aliasing rules in the C++ spec, or do you need to use unions more?

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That said, why do we even want this approach?  Couldn't we just change the definition of PRUnichar, get the benefits of this more broadly, and have less messy code?

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(And if that turns out to be fine, then we can get rid of the PRUnichar name and switch to the char16_t name.)

Comment 4

[Joshua Cranmer [:jcranmer]]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #1)
> Could you add static_asserts that char16_t is unsigned and has sizeof() ==
> 2?  (I think we have those for PRUnichar somewhere.)

We have those in mozilla/Char16.h, I believe.

> Is this valid in terms of the aliasing rules in the C++ spec, or do you need
> to use unions more?

This violates strict aliasing. However, we already compile with -fno-strict-aliasing, and I expect this to be a temporary measure (see bug 895047), so I'm not too concerned about it.

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #2)
> That said, why do we even want this approach?  Couldn't we just change the
> definition of PRUnichar, get the benefits of this more broadly, and have
> less messy code?

A mass-conversion of PRUnichar->char16_t I think is riskier than any of the other stdint types. I want to ease in the use of char16_t a bit more gradually. That said, mixing char16_t and PRUnichar quickly becomes extremely painful--if we want to migrate jschar to char16_t independently of each other, then we are likely going to need to make nsString simultaneously supporting PRUnichar and char16_t for the transition period. :-(

My basic transition plan is shaping up as follows:
1. Use mozilla/Char16.h in place of our current configure-driven macros
2. Make nsString somewhat usable with either char16_t or PRUnichar
3. Change the typedef of jschar to char16_t.
4. Change-or-sed PRUnichar to char16_t (the typedef approach is looking less viable).
5. Mass-kill dumb string APIs (basically, get rid of all NS_*LITERAL*STRING).

Comment 5

[Joshua Cranmer [:jcranmer]]

(The main problem with trying to just typedef char16_t PRUnichar; is that it breaks anyone who includes prtypes.h after nscore.h. Also, nscore.h can ostensibly be included by C code, and Char16.h doesn't work for C yet...).

Comment 6

[Justin Lebar (not reading bugmail)]

Comment on attachment 784756 [details] [diff] [review]
string-uses-char16

I can look closely at this, I'd like dbaron's blessing on the plan.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Joshua Cranmer [:jcranmer] from comment #4)
> (In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from
> comment #1)
> > Could you add static_asserts that char16_t is unsigned and has sizeof() ==
> > 2?  (I think we have those for PRUnichar somewhere.)
> 
> We have those in mozilla/Char16.h, I believe.

Not the unsignedness one, which is perhaps the most important.

> A mass-conversion of PRUnichar->char16_t I think is riskier than any of the
> other stdint types. I want to ease in the use of char16_t a bit more
> gradually. That said, mixing char16_t and PRUnichar quickly becomes
> extremely painful--if we want to migrate jschar to char16_t independently of
> each other, then we are likely going to need to make nsString simultaneously
> supporting PRUnichar and char16_t for the transition period. :-(

So jschar and PRUnichar certainly used to be incompatible; I remember having to add casts for that incompatibility.  And looking at jspubtd.h they still look incompatible.

> My basic transition plan is shaping up as follows:
> 1. Use mozilla/Char16.h in place of our current configure-driven macros
> 2. Make nsString somewhat usable with either char16_t or PRUnichar
> 3. Change the typedef of jschar to char16_t.
> 4. Change-or-sed PRUnichar to char16_t (the typedef approach is looking less
> viable).

I'm certainly more comfortable with the changes in this patch being temporary than permanent, but if jschar and PRUnichar are already incompatible (which I think they are), then it's not clear to me that it's even needed.

But if you still think it's needed, I'm ok with it being temporary.

> 5. Mass-kill dumb string APIs (basically, get rid of all NS_*LITERAL*STRING).

I'd certainly be in favor of de-macro-izing it.  Are you talking about that, or more?

Comment 8

[Joshua Cranmer [:jcranmer]]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #7)
> (In reply to Joshua Cranmer [:jcranmer] from comment #4)
> > (In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from
> > comment #1)
> > > Could you add static_asserts that char16_t is unsigned and has sizeof() ==
> > > 2?  (I think we have those for PRUnichar somewhere.)
> > 
> > We have those in mozilla/Char16.h, I believe.
> 
> Not the unsignedness one, which is perhaps the most important.

Not hard to add.

> So jschar and PRUnichar certainly used to be incompatible; I remember having
> to add casts for that incompatibility.  And looking at jspubtd.h they still
> look incompatible.

Changing jschar to char16_t broke my build at least, but if we're fine saying that jschar and PRUnichar can be incompatible, I can certainly try that first.

> > 5. Mass-kill dumb string APIs (basically, get rid of all NS_*LITERAL*STRING).
> 
> I'd certainly be in favor of de-macro-izing it.  Are you talking about that,
> or more?

Getting rid of NS_LITERAL_STRING("").get() is the biggest thing I want to do. If we add string-literal constructors to nsC?String and nsDependentC?String, we can basically eliminate the other macros too.

Comment 9

[Justin Lebar (not reading bugmail)]

Do you still want me to look at this patch, or should I wait for a new version?

Comment 10

[Joshua Cranmer [:jcranmer]]

Comment on attachment 784756 [details] [diff] [review]
string-uses-char16

No, I'll try tackling jschar first.

Comment 11

[(no longer active)]

I think we should do bug 895047 instead.