904985 - Use Char16.h in lieu of NS_LL

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Joshua Cranmer [:jcranmer]]

Attachment: Part 1: Better support for char16_t in mfbt

The easiest way to use mozilla/Char16.h without doing a bitrotty sed change is to force-include it early and drop our old attempts.

[David Baron mentioned he wanted an unsignedness check for char16_t, so I'm adding that in part 1. It's not strictly necessary for the rest of the code, but if we want to start using it for jschar or PRUnichar, we need it.]

Comment 1

[Joshua Cranmer [:jcranmer]]

Attachment: Use Char16.h for NS_LITERAL_STRING and friends

I tested this much on try, and it at least builds: <https://tbpl.mozilla.org/?tree=Try&rev=1b2c20962f8e>.

And we get to lose -fshort-wchar and *two* configure checks! \o/

Comment 2

[Joshua Cranmer [:jcranmer]]

Attachment: Part 3: Kill NS_LL

MOZ_UTF16 obviates the need for it. Actually, we could probably kill the distinction between multiline and not multiline, since MOZ_UTF16("a") "b" should be equivalent to MOZ_UTF16("ab")...

Comment 3

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

Comment on attachment 789984 [details] [diff] [review]
Use Char16.h for NS_LITERAL_STRING and friends

nscore.h:  It's not clear to me which option we want for gcc on
Windows.  Maybe it doesn't matter -- but at least maybe leave a comment
that maybe it should use wchar_t, unless it's obvious to you one way or
the other which one it currently uses.  (I'm not in the mood to figure
out whether it runs the autoconf tests.)

Why does nscore.h need to include Char16.h?  (You need it in nsStringAPI.h
and nsLiteralString.h though.)

nsStaticAtom:  Could you just remove the NS_STATIC_ATOM_USE_WIDE_STRINGS macro
and remove the unused half of the ifdef?

nsStringAPI.h:  need to fix the comment above the code you're changing


Otherwise looks good.  Though I suppose you need a build peer's review on the configure.in changes?

Comment 4

[Joshua Cranmer [:jcranmer]]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #3)
> nscore.h:  It's not clear to me which option we want for gcc on
> Windows.  Maybe it doesn't matter -- but at least maybe leave a comment
> that maybe it should use wchar_t, unless it's obvious to you one way or
> the other which one it currently uses.  (I'm not in the mood to figure
> out whether it runs the autoconf tests.)

I was under the impression that mingw-gcc was using uint16_t for PRUnichar, although on further reflection, it's not, so I'll go change that header include.

> Why does nscore.h need to include Char16.h?  (You need it in nsStringAPI.h
> and nsLiteralString.h though.)

I wanted to flesh out any problems with using char16_t globally early on. It doesn't look like there's an easy way to use char16_t instead of PRUnichar without sed'ing it globally, and making jschar and PRUnichar not be the same underlying type has turned out to be a code-rewriting headache. (Granted, nsStringAPI.h/nsLiteralString.h are pretty global headers themselves...)

> nsStaticAtom:  Could you just remove the NS_STATIC_ATOM_USE_WIDE_STRINGS
> macro
> and remove the unused half of the ifdef?

Er, yes, I could, thanks for remidning me.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 789980 [details] [diff] [review]
Part 1: Better support for char16_t in mfbt

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

::: mfbt/Char16.h
@@ +39,5 @@
> +   /**
> +    * This macro is used to distinguish when char16_t would be a distinct
> +    * typedef from wchar_t.
> +    */
> +#  define CHAR16_IS_NOT_WCHAR

MOZ_CHAR16_IS_NOT_WCHAR, please.

Comment 6

[Joshua Cranmer [:jcranmer]]

Attachment: Part 2: Use Char16.h for NS_LITERAL_STRING and friends

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 791972 [details] [diff] [review]
Part 2: Use Char16.h for NS_LITERAL_STRING and friends

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

::: xpcom/base/nscore.h
@@ +334,4 @@
>      typedef wchar_t PRUnichar;
>    #else
>      typedef uint16_t PRUnichar;
>    #endif

Any reason not to use char16_t there as well?

Comment 8

[Joshua Cranmer [:jcranmer]]

(In reply to Mike Hommey [:glandium] from comment #7)
> Comment on attachment 791972 [details] [diff] [review]
> Part 2: Use Char16.h for NS_LITERAL_STRING and friends
> 
> Review of attachment 791972 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/base/nscore.h
> @@ +334,4 @@
> >      typedef wchar_t PRUnichar;
> >    #else
> >      typedef uint16_t PRUnichar;
> >    #endif
> 
> Any reason not to use char16_t there as well?

Typedef'ing char16_t to PRUnichar at this point breaks _a lot_ of code.

Comment 9

[Mike Hommey [:glandium]]

(In reply to Joshua Cranmer [:jcranmer] from comment #8)
> Typedef'ing char16_t to PRUnichar at this point breaks _a lot_ of code.

You should be able to do it at least on windows. (Char16.h actually makes it mean wchar_t)

Comment 10

[Joshua Cranmer [:jcranmer]]

(In reply to Mike Hommey [:glandium] from comment #9)
> (In reply to Joshua Cranmer [:jcranmer] from comment #8)
> > Typedef'ing char16_t to PRUnichar at this point breaks _a lot_ of code.
> 
> You should be able to do it at least on windows. (Char16.h actually makes it
> mean wchar_t)

The char16_t define in Char16.h is keyed off of _MSC_VER IIRC, this one is on _WIN32. mingw gcc is going to get screwed (I don't know what the correct solution is there).

Comment 11

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

Comment on attachment 791972 [details] [diff] [review]
Part 2: Use Char16.h for NS_LITERAL_STRING and friends

>diff --git a/xpcom/glue/nsStringAPI.h b/xpcom/glue/nsStringAPI.h
>+static_assert(sizeof(char16_t) == 2, "size of char16_t must be 2");

Could you also toss in the static_assert that char16_t(-1) > char16_t(0), "char16_t must be unsigned"


And could you add the same static_assert in nsString.h for both PRUnichar and nsString::char_type (for the internal API), next to the assertions about being size 2?

r=dbaron with that, and sorry about the delay

Comment 12

[Joshua Cranmer [:jcranmer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/11af1e7c053f
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a62484ff46b
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0479039a286

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/11af1e7c053f
https://hg.mozilla.org/mozilla-central/rev/0a62484ff46b
https://hg.mozilla.org/mozilla-central/rev/b0479039a286