Closed Bug 870698 Opened 7 years ago Closed 2 years ago

Replace .Append/Assign("...") with .Append/AssignLiteral("...")

Categories

(Core :: String, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: alfredkayser, Assigned: cpeterson)

Details

Attachments

(11 files)

21.05 KB, patch
erahm
: review+
Details | Diff | Splinter Review
58.73 KB, patch
erahm
: review+
Details | Diff | Splinter Review
8.97 KB, patch
erahm
: review+
Details | Diff | Splinter Review
24.00 KB, patch
erahm
: review+
Details | Diff | Splinter Review
2.50 KB, patch
erahm
: review+
Details | Diff | Splinter Review
10.20 KB, patch
erahm
: review+
Details | Diff | Splinter Review
9.98 KB, patch
erahm
: review+
Details | Diff | Splinter Review
19.31 KB, patch
erahm
: review+
Details | Diff | Splinter Review
9.95 KB, patch
erahm
: review+
Details | Diff | Splinter Review
15.23 KB, patch
erahm
: review+
Details | Diff | Splinter Review
8.92 KB, patch
erahm
: review+
Details | Diff | Splinter Review
string.Append("...") is to be replaced by string.AppendLiteral("...")
AppendLiteral uses templating to prevent strlen on the static string.

.Append("..."):
http://mxr.mozilla.org/mozilla-central/search?string=.Append%28%22&case=on&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

.Assign("..."):
http://mxr.mozilla.org/mozilla-central/search?string=.Assign%28%22&case=1&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
really wouldn't it be better to just replace  all the void AssignX(T string) with operator =(T string)?  and similar for AppendX() with operator +=?
The AssignLiteral passes the string length of the constant string instead of calculating it later:
375       template<int N>
376       void AssignLiteral( const char (&str)[N] )
377                   { AssignASCII(str, N-1); }

This doesn't happen with =/+=.
(In reply to Alfred Kayser from comment #2)
> The AssignLiteral passes the string length of the constant string instead of
> calculating it later:
> 375       template<int N>
> 376       void AssignLiteral( const char (&str)[N] )
> 377                   { AssignASCII(str, N-1); }
> 
> This doesn't happen with =/+=.


yes, I know, but we should just change that (I'm pretty sure the same tempate trick works with operators / constructors)
(In reply to Trevor Saunders from comment #3)
> (In reply to Alfred Kayser from comment #2)
> > The AssignLiteral passes the string length of the constant string instead of
> > calculating it later:
> > 375       template<int N>
> > 376       void AssignLiteral( const char (&str)[N] )
> > 377                   { AssignASCII(str, N-1); }
> > 
> > This doesn't happen with =/+=.
> 
> yes, I know, but we should just change that (I'm pretty sure the same
> tempate trick works with operators / constructors)

Sure, but the const char* overload is preferred over the const char (&)[N] template, which is why we have to give the *Literal methods their own names.
Assignee: alfredkayser → cpeterson
Component: XPCOM → String
MozReview-Commit-ID: A0u9PP49OW3
Attachment #8906895 - Flags: review?(erahm)
MozReview-Commit-ID: CrkIP4iHP1U
Attachment #8906896 - Flags: review?(erahm)
MozReview-Commit-ID: 7ERQfcVAiRx
Attachment #8906898 - Flags: review?(erahm)
MozReview-Commit-ID: G1GhyvD29WK
Attachment #8906899 - Flags: review?(erahm)
The NS_LITERAL_CSTRING macro creates a temporary nsLiteralCString to encapsulate the string literal and its length, but AssignLiteral() can determine the string literal's length at compile-time without nsLiteralCString.

MozReview-Commit-ID: KXJM13VRTB7
Attachment #8906900 - Flags: review?(erahm)
The NS_LITERAL_CSTRING macro creates a temporary nsLiteralCString to encapsulate the string literal and its length, but AssignLiteral() can determine the string literal's length at compile-time without nsLiteralCString.

MozReview-Commit-ID: F750v6NN81s
Attachment #8906901 - Flags: review?(erahm)
The NS_LITERAL_CSTRING macro creates a temporary nsLiteralCString to encapsulate the string literal and its length, but AssignLiteral() can determine the string literal's length at compile-time without nsLiteralCString.

MozReview-Commit-ID: DbTW5Bhd9E1
Attachment #8906902 - Flags: review?(erahm)
The NS_LITERAL_CSTRING macro creates a temporary nsLiteralCString to encapsulate the string literal and its length, but AssignLiteral() can determine the string literal's length at compile-time without nsLiteralCString.

MozReview-Commit-ID: B5Y8KyExPQ8
Attachment #8906903 - Flags: review?(erahm)
The NS_LITERAL_STRING macro creates a temporary nsLiteralString to encapsulate the char16_t string literal and its length, but AssignLiteral() can determine the char16_t string literal's length at compile-time without nsLiteralString.

MozReview-Commit-ID: 6vgQiU8zN3o
Attachment #8906904 - Flags: review?(erahm)
The NS_LITERAL_STRING macro creates a temporary nsLiteralString to encapsulate the char16_t string literal and its length, but AssignLiteral() can determine the char16_t string literal's length at compile-time without nsLiteralString.

MozReview-Commit-ID: H9I6vNDMdIr
Attachment #8906905 - Flags: review?(erahm)
The NS_LITERAL_STRING macro creates a temporary nsLiteralString to encapsulate the char16_t string literal and its length, but AssignLiteral() can determine the char16_t string literal's length at compile-time without nsLiteralString.

MozReview-Commit-ID: L9UE3gXHG4Q
Attachment #8906906 - Flags: review?(erahm)
* Some calls to AssignLiteral/AppendLiteral(".") pass string literal arguments that are just one character long (plus the NUL terminator byte). Is it worth replacing these with calls to Assign/Append('.') with char literal arguments to avoid some of the string-handling overhead of AssignLiteral/AppendLiteral()?

* I did not replace calls to `Equals(NS_LITERAL_STRING("..."))` with `EqualsLiteral(u"...")` because there is no EqualLiteral() function overloaded for char16_t string literals. Is it worth adding one for symmetry? There are only about 10 calls to `Equals(NS_LITERAL_STRING("..."))`.

* There are many calls to AssignLiteral/AppendLiteral("...") with incompatible_char_type string literals. Is it worth fixing these callers to pass compatible char_type string literals so AssignLiteral/AppendLiteral(u"...") can use a fast memcpy instead of a char-by-char copy loop? There are thousands of these incompatible_char_type calls. :(
Attachment #8906895 - Flags: review?(erahm) → review+
Attachment #8906896 - Flags: review?(erahm) → review+
Attachment #8906898 - Flags: review?(erahm) → review+
Attachment #8906899 - Flags: review?(erahm) → review+
Comment on attachment 8906900 [details] [diff] [review]
Part 5: Replace Assign(NS_LITERAL_CSTRING("")) with AssignLiteral("")

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

It's also less verbose and loud which is nice :)
Attachment #8906900 - Flags: review?(erahm) → review+
Attachment #8906901 - Flags: review?(erahm) → review+
Attachment #8906902 - Flags: review?(erahm) → review+
Attachment #8906903 - Flags: review?(erahm) → review+
Attachment #8906904 - Flags: review?(erahm) → review+
Attachment #8906905 - Flags: review?(erahm) → review+
Attachment #8906906 - Flags: review?(erahm) → review+
It seems like as a follow up we might want to put static analysis in place that yells if we're calling Assign/Append/Insert/Equals w/ a `nsTLiteralString`
Thanks! I'll see if we can an analysis or add deleted member function overloads for nsTLiteralString to prevent people from calling them.

Because these patches touch a lot of files, I will wait to land them when Nightly 58 starts next week to avoid affecting Firefox 57 stability.
Pushed by cpeterson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/947050c037c7
Part 1: Replace Assign("") with AssignLiteral(""). r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/c81b52d58ea4
Part 2: Replace Append("") with AppendLiteral(""). r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e2092020124
Part 3: Replace Insert("") with InsertLiteral(""). r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/245dfda695c2
Part 4: Replace Equals("") with EqualsLiteral(""). r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/231c993f7ea3
Part 5: Replace Assign(NS_LITERAL_CSTRING("")) with AssignLiteral(""). r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/2725a1e0b35c
Part 6: Replace Append(NS_LITERAL_CSTRING("")) with AppendLiteral(""). r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/072afbaf1033
Part 7: Replace Insert(NS_LITERAL_CSTRING("")) with InsertLiteral(""). r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/81565c99c596
Part 8: Replace Equals(NS_LITERAL_CSTRING("")) with EqualsLiteral(""). r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5b71c927db1
Part 9: Replace Assign(NS_LITERAL_STRING("")) with AssignLiteral(u""). r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/3857318b8a88
Part 10: Replace Append(NS_LITERAL_STRING("")) with AppendLiteral(u""). r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/39ac3246ab5c
Part 11: Replace Insert(NS_LITERAL_STRING("")) with InsertLiteral(u""). r=erahm
Two questions from Thunderbird:
1) We should do the same in C-C code, right?
2) Any plans to disallow the constructs which you replaced now?
   Looks like it, see comment #19. Please CC me on that bug.
(In reply to Jorg K (GMT+2) from comment #22)
> Two questions from Thunderbird:
> 1) We should do the same in C-C code, right?

These changes are optional. They are just a small optimization.

> 2) Any plans to disallow the constructs which you replaced now?
>    Looks like it, see comment #19. Please CC me on that bug.

I don't think it is worth disallowing the existing usage. They are not wrong. They're just missing a small optimization. I don't currently plan to implement warnings or errors for the existing usage, but if I do later, I will make that bug block this bug so you should see the Bugzilla bug mail about it.
(In reply to Chris Peterson [:cpeterson] from comment #16)
> * Some calls to AssignLiteral/AppendLiteral(".") pass string literal
> arguments that are just one character long (plus the NUL terminator byte).
> Is it worth replacing these with calls to Assign/Append('.') with char
> literal arguments to avoid some of the string-handling overhead of
> AssignLiteral/AppendLiteral()?

Neil (now long gone) also thinks Append('.') is faster than AppendLiteral(".").

Is there a special code patch in Append for char arguments to make it still faster than AppendLiteral?
Another question. I've been looking into this in bug 1402750.

I've noticed that we have AppendLiteral() to UTF-16 strings without the u"xxx", for example:
https://dxr.mozilla.org/comm-central/rev/f17b4b49ffa6fce2737f0827548995a692480d68/mailnews/base/util/nsMsgProtocol.cpp#367

So is the 'u' optional?
Flags: needinfo?(cpeterson)
Sorry, the "u" question is answered in comment #16, so it works without the "u" but is more efficient with it.

I see you didn't go for the single character function: Assign('[');
https://hg.mozilla.org/integration/mozilla-inbound/rev/947050c037c7#l4.23
(In reply to :aceman from comment #24)
> Neil (now long gone) also thinks Append('.') is faster than
> AppendLiteral(".").
> 
> Is there a special code patch in Append for char arguments to make it still
> faster than AppendLiteral?

Append('.') has a shorter code path and calls fewer functions than AppendLiteral("."). I can look into changing AppendLiteral(".") calls to Append('.') and/or adding a new overloaded template function when AppendLiteral() is called with a string of length 1.


(In reply to Jorg K (GMT+2) from comment #26)
> Sorry, the "u" question is answered in comment #16, so it works without the
> "u" but is more efficient with it.

It depends on whether the string object being modified is an nsCString (8-bit char) or nsString (UTF-16 char16_t). The various ns*String classes have overloaded member functions for assigning or appending either string types, but their usage seems inconsistent in different files.

In my patches, I directly replaced the original code with equivalent code to avoid changing the behavior. For example, the NS_LITERAL_CSTRING macro produces an 8-bit char string literal and the NS_LITERAL_STRING macro produces a char16_t string literal.


> I see you didn't go for the single character function: Assign('[');
> https://hg.mozilla.org/integration/mozilla-inbound/rev/947050c037c7#l4.23

I considered it, but because these patches change lots of files, I wanted them to be direct replacements of Append("") to AppendLiteral("") to make code review easier.
Flags: needinfo?(cpeterson)
Thanks. I would vote for implementing the shortcut/template for Append/AssignLiteral so that people no longer need to wonder why single chars still use Append/Assign. Having all literals use Append/AssignLiteral would be cleanest.
In bug 1402750 I've shuffled all the calls with literal strings to the *Literal() functions. Only the one-character literals I shuffled to Append('x') or Assign('x'). All of C-C should be consistent now.

I'd be interested to find literals missing the "u" when used for an nsString. I assume using a "u" for an nsCString won't work.
We're still doing a bit of clean-up in comm-central. Would it make sense to
-  SetProcessName(NS_LITERAL_STRING("Large Allocation Web Content"));
+  SetProcessName(NS_LITERAL_STRING(u"Large Allocation Web Content"));
throughout?

mozilla-central uses NS_LITERAL_STRING(u"xxx"); on very few occasions although, unless I'm missing something, it could/should be used everywhere.
Flags: needinfo?(cpeterson)
(In reply to Jorg K (GMT+2) from comment #30)
> We're still doing a bit of clean-up in comm-central. Would it make sense to
> -  SetProcessName(NS_LITERAL_STRING("Large Allocation Web Content"));
> +  SetProcessName(NS_LITERAL_STRING(u"Large Allocation Web Content"));
> throughout?

You don't need to change NS_LITERAL_STRING("") to NS_LITERAL_STRING(u""). The NS_LITERAL_STRING's macro uses string literal concatenation to add the `u` prefix for you. :)

  #define NS_LITERAL_STRING(s) static_cast<const nsLiteralString&>(nsLiteralString(u"" s))
                                                                                   ^^^
Flags: needinfo?(cpeterson)
You need to log in before you can comment on or make changes to this bug.