1593666 - Specify preferred way to construct literal empty and non-empty strings

Status: NEW

(Core :: DOM: File, task, P3)

Description

[Simon Giesecke [:sg] [he/him]]

There are several different variants to instantiate an empty C-string (analogously for a 16-bit string):

• NS_LITERAL_CSTRING("")
• nsLiteralCString("")
• EmptyCString()
• nsCString() (not entirely sure if this is equivalent, this is not well-documented)
• nsCString("")
• nsDependentCString("")

Only the first two of these are constexpr, and allows for exploiting their literal-ness at compile time by custom overloads of functions/operators for nsLiteral[C]String (without needing to rely on compiler optimizations, which are never done in debug builds, and probably will also not be done in release builds given the complexity of ns[C]String).

Since the NS_LITERAL_[C]STRING macros exist on purpose to avoid accidental misuse of nsLiteral[C]String with non-literals, I suggest to recommend the use of NS_LITERAL_CSTRING("") by default.

For non-empty literal strings the benefits of NS_LITERAL_[C]STRING are even more obvious (it is not necessary to calculate the length at run-time, and no copies of the string data are needed), and so the macros should be used ubiquitously in those cases.

In addition, NS_NAMED_LITERAL_[C]STRING(name, "foo"); should be discouraged in favor of constexpr auto foo = NS_LITERAL_[C]STRING("foo"); (reducing the non-standard syntax to the necessary minimum).

I am not sure if there are situations at all where using Empty[C]String() would provide significant benefits over NS_LITERAL_[C]STRING(""). Theoretically, this might happen when the conversion from a nsLiteral[C]String to a ns[C]String occurs which is not optimized away. However, such a conversion will not always be necessary, since there is custom handling of nsLiteral[C]String in some contexts.

Side remark (which, if found worthwhile to pursue, should be raised as an additional bug for the String component):
Based on this, it could be beneficial to change Empty[C]String to be constexpr and return a nsLiteral[C]String (by value) instead of returning a const reference to a ns[C]String. A function returning a ns[C]String by value or reference cannot be constexpr, since nsTSubstring has a user-provided destructor, which can probably not be eliminated.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

Agreed on:

• Explicitly providing guidance here. https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings is great but verbose, and NS_LITERAL_* is what https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings#Literal_Strings indicates to use.
• NS_NAMED_LITERAL_* is weird and awkward and suggesting a non-awkward way is great. It might make sense to try and flow some of that guidance back into the page above, but it's not a necessity as it may result in a bit o fbike-shedding.
• Good to help optimize Empty[C]String() and provide clarity about it. I do personally like it over NS_LITERAL_STRING("") both from a readability perspective and from a "works better for auditing empty versus void strings using searchfox" perspective.

Thanks for your continued efforts and leadership here!

Comment 2

[Simon Giesecke [:sg] [he/him]]

I opened bug 1594389 for modifying/amending Empty[C]String.