If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Use Char16.h in lieu of NS_LL

RESOLVED FIXED in mozilla27

Status

()

Core
String
RESOLVED FIXED
4 years ago
a year ago

People

(Reporter: jcranmer, Assigned: jcranmer)

Tracking

Trunk
mozilla27
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
Created attachment 789980 [details] [diff] [review]
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.]
Attachment #789980 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 1

4 years ago
Created attachment 789984 [details] [diff] [review]
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/
Attachment #789984 - Flags: review?(dbaron)
(Assignee)

Comment 2

4 years ago
Created attachment 789985 [details] [diff] [review]
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")...
Attachment #789985 - Flags: review?(dbaron)
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?
Attachment #789985 - Flags: review?(dbaron) → review+
(Assignee)

Comment 4

4 years ago
(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 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.
Attachment #789980 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 6

4 years ago
Created attachment 791972 [details] [diff] [review]
Part 2: Use Char16.h for NS_LITERAL_STRING and friends
Attachment #789984 - Attachment is obsolete: true
Attachment #789984 - Flags: review?(dbaron)
Attachment #791972 - Flags: review?(mh+mozilla)
Attachment #791972 - Flags: review?(dbaron)
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?
Attachment #791972 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 8

4 years ago
(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.
(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)
(Assignee)

Comment 10

4 years ago
(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).
OS: Linux → All
Hardware: x86_64 → All
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
Attachment #791972 - Flags: review?(dbaron) → review+
(Assignee)

Comment 12

4 years ago
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
https://hg.mozilla.org/mozilla-central/rev/11af1e7c053f
https://hg.mozilla.org/mozilla-central/rev/0a62484ff46b
https://hg.mozilla.org/mozilla-central/rev/b0479039a286
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Depends on: 1277106
You need to log in before you can comment on or make changes to this bug.