Closed Bug 802469 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
Attached patch Patch? v1 (obsolete) — Splinter Review
Assignee: nobody → justin.lebar+bug
Attached patch Patch v1Splinter Review
Attachment #672302 - Flags: review?(benjamin)
Attachment #672156 - Attachment is obsolete: true
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/jlebar@mozilla.com-94ad4e353e1a
Comment on attachment 672302 [details] [diff] [review]
Patch v1

Why introduce the internal namespace? Can't those functions just live in mozilla:: ?
(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.
Attachment #672302 - Flags: review?(benjamin) → review+
https://hg.mozilla.org/mozilla-central/rev/dd21d2520008
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Component: String → XPCOM
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: