802469 - Make it a compile error to pass a non-literal to NS_LITERAL_{,C}STRING

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Justin Lebar (not reading bugmail)]

Right now, if you pass a non-literal to NS_LITERAL_{C,}STRING, your code may compile.  But it will not behave properly.

I think I have a trick to make this error a compile-time error.  Not to keep you in suspense unnecessarily, but let me write the patch.

Comment 1

[Justin Lebar (not reading bugmail)]

This was not what I initially had in mind, but I think it works.  https://tbpl.mozilla.org/?tree=Try&rev=94ad4e353e1a

Misuse of NS_LITERAL_STRING was already a compile error anywhere that we have wide literal strings, because NS_LITERAL_STRING("foo") expanded in part to |u"foo"| or |L"foo"|, which doesn't work if "foo" isn't a literal.  But NS_LITERAL_CSTRING has no such constraint.

While I'm here, I'd really like to make it possible to do

  NS_LITERAL_STRING("foo" "bar").

Right now this doesn't work because we append the "u" or "L", and we have (u"foo" "bar"), which fails to compile because we're appending a wide string with a narrow string.  I tried making the macro do u("foo" "bar"), but that fails to compile too.

I guess we could make NS_LITERAL_STRING a function in order to make this work?  I didn't do that because my C++-fu is not strong enough to understand the implications of doing so.

Comment 2

[Justin Lebar (not reading bugmail)]

Attachment: Patch? v1

Comment 3

[Justin Lebar (not reading bugmail)]

Fix for a Windows typo: https://tbpl.mozilla.org/?tree=Try&rev=10526198bc43

Comment 4

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

Comment 5

[Mozilla RelEng Bot]

Try run for 94ad4e353e1a is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=94ad4e353e1a
Results (out of 187 total builds):
    success: 172
    warnings: 12
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-94ad4e353e1a

Comment 6

[Benjamin Smedberg]

Comment on attachment 672302 [details] [diff] [review]
Patch v1

Why introduce the internal namespace? Can't those functions just live in mozilla:: ?

Comment 7

[Justin Lebar (not reading bugmail)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)
> Comment on attachment 672302 [details] [diff] [review]
> Patch v1
> 
> Why introduce the internal namespace? Can't those functions just live in
> mozilla:: ?

I didn't want to expose a new global function, because I didn't think anyone else would care to use it.  (It is also, as currently written, tricky to call GetWStringLength correctly, since it doesn't always exist.)

I'd rather do mozilla::internal_to_literalstrings_h (or whatever), but I understood the consensus of a newsgroup thread some months ago to be that we'd use mozilla::internal for this sort of thing.

I'm happy to move the functions to mozilla:: if you think that would be better.

Comment 8

[Justin Lebar (not reading bugmail)]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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