MOZ_UTF16() macro needs intermediary macro to handle stringification and concatenation

RESOLVED FIXED in Firefox 20

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

Trunk
mozilla20
Points:
---

Firefox Tracking Flags

(firefox19 wontfix, firefox20 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
Created attachment 697300 [details] [diff] [review]
fix-MOZ_UTF16-stringification.patch

As described in nsStringAPI.h, the NS_LL macro needed an intermediary macro (NS_L) to correctly handle stringification and concatenation of macro parameters to NS_LL. Char16.h's MOZ_UTF16() macro needs to do the same thing.

https://hg.mozilla.org/mozilla-central/file/4e18ac9b51e2/xpcom/glue/nsStringAPI.h#l1105

I chose the name `MOZ_UTF16_()` for the intermediary macro because it's a private implementation detail of MOZ_UTF16() and AFAIK does not need to be called by other code. I am open to suggestions for better names. <:)

Note that comm-central has a few uses of NS_L instead of NS_LL, but I don't see why they couldn't just use NS_LL:

https://mxr.mozilla.org/comm-central/ident?i=NS_L
Attachment #697300 - Flags: review?(jwalden+bmo)
What about "MOZ_u" for the public name? It looks more close to "u" prefix.
(Assignee)

Comment 2

6 years ago
(In reply to Masatoshi Kimura [:emk] from comment #1)
> What about "MOZ_u" for the public name? It looks more close to "u" prefix.

"MOZ_u" sounds OK to me, though macro naming conventions prefer uppercase names. "MOZ_U", however, would be a bad name because it looks like C++11's U"" prefix for UTF-32 string literals.

Comment 3

6 years ago
Comment on attachment 697300 [details] [diff] [review]
fix-MOZ_UTF16-stringification.patch

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

::: mfbt/Char16.h
@@ +27,5 @@
>      * to Windows's 16-bit wchar_t so we can declare UTF-16 literals as constant
>      * expressions (and pass char16_t pointers to Windows APIs). We #define our
>      * char16_t as a macro to override yval.h's typedef of the same name.
>      */
> +#  define MOZ_UTF16_(s) L##s

I think we usually use a _HELPER suffix for macros like this, so make this MOZ_UTF16_HELPER.  Such a name is also less amenable to typos that might use this macro outside its one intended use in this file.
Attachment #697300 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 4

6 years ago
waldo: do you have an opinion on the public macro's name "MOZ_UTF16" versus emk's "MOZ_u" suggestion?

Comment 5

6 years ago
MOZ_UTF16 seems more readable to me than MOZ_u, plus it's what we already have, so I'd stick with that name.
(Assignee)

Comment 6

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d05c7020797
status-firefox19: --- → wontfix
status-firefox20: --- → fixed
Target Milestone: --- → mozilla20
https://hg.mozilla.org/mozilla-central/rev/5d05c7020797
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.