[DevTools] Reduce API surface of String16.

[DevTools] Reduce API surface of String16.

Also, do not mention WTF::String anywhere except for String16WTF.
Drive-by: improved performance of String16Builder.

BUG=580337
Committed: https://crrev.com/16523d99c644c8513ad586eaca935fa2bea877b1
Cr-Commit-Position: refs/heads/master@{#411882}

[dgozman, 2016-08-09 01:34:20]

dgozman@chromium.org changed reviewers:
+ eostroukhov@chromium.org, kozyatinskiy@chromium.org, pfeldman@chromium.org

[dgozman, 2016-08-09 01:34:21]

Take a look please.

[kozy, 2016-08-09 16:40:37]

https://codereview.chromium.org/2226863003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/inspector_protocol/String16STL.h
(right):

https://codereview.chromium.org/2226863003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/inspector_protocol/String16STL.h:115: // This
class should not be ever instantiated, so we don't implement constructors.
Can we make constructor private?

https://codereview.chromium.org/2226863003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/v8_inspector/V8StringUtil.cpp (right):

https://codereview.chromium.org/2226863003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/v8_inspector/V8StringUtil.cpp:104: String16
lineEndString = "\n";
const String16

[pfeldman, 2016-08-09 17:20:39]

https://codereview.chromium.org/2226863003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/inspector_protocol/String16.h (right):

https://codereview.chromium.org/2226863003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/inspector_protocol/String16.h:21:
PLATFORM_EXPORT bool isASCIISpace(UChar);
Looks like String16Base others inherit from to me.

https://codereview.chromium.org/2226863003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/inspector_protocol/String16STL.h
(right):

https://codereview.chromium.org/2226863003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/inspector_protocol/String16STL.h:57:
std::size_t hash() const
Could this also move out?

https://codereview.chromium.org/2226863003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/inspector_protocol/String16STL.h:76: inline
bool operator==(const String16& a, const String16& b) { return a.impl() ==
b.impl(); }
Same if you make base a template with pre-defined impl.

[dgozman, 2016-08-10 23:21:50]

PTAL, addressed all comments.

[dgozman, 2016-08-10 23:21:54]

The CQ bit was checked by dgozman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 23:22:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 00:05:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-11 00:05:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pfeldman, 2016-08-11 00:30:32]

https://codereview.chromium.org/2226863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/inspector_protocol/String16.cpp (right):

https://codereview.chromium.org/2226863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/inspector_protocol/String16.cpp:18:
std::sprintf(buffer, "%d", number);
Please check whether we can use snprintf in node, If we can, use it with the
length passed here. Otherwise, do something to ensure buffer is allocated.

[dgozman, 2016-08-11 00:33:37]

https://codereview.chromium.org/2226863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/inspector_protocol/String16.cpp (right):

https://codereview.chromium.org/2226863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/inspector_protocol/String16.cpp:18:
std::sprintf(buffer, "%d", number);
On 2016/08/11 00:30:32, pfeldman wrote:
> Please check whether we can use snprintf in node, If we can, use it with the
> length passed here. Otherwise, do something to ensure buffer is allocated.

But buffer is allocated on the callsite. This is internal-only function.

[dgozman, 2016-08-11 02:59:53]

PTAL

https://codereview.chromium.org/2226863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/inspector_protocol/String16.cpp (right):

https://codereview.chromium.org/2226863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/inspector_protocol/String16.cpp:18:
std::sprintf(buffer, "%d", number);
On 2016/08/11 00:30:32, pfeldman wrote:
> Please check whether we can use snprintf in node, If we can, use it with the
> length passed here. Otherwise, do something to ensure buffer is allocated.

Using snprintf.

[dgozman, 2016-08-11 03:00:01]

The CQ bit was checked by dgozman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-11 03:00:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 03:15:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-11 03:15:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pfeldman, 2016-08-12 17:41:34]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2016-08-12 17:41:35]

lgtm

[commit-bot: I haz the power, 2016-08-12 17:42:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 18:04:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-12 18:04:45]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[kozy, 2016-08-12 18:17:37]

lgtm

[dgozman, 2016-08-12 18:45:27]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2016-08-12 18:45:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
kozyatinskiy@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2226863003/#ps100001
(title: "win compile")

[commit-bot: I haz the power, 2016-08-12 18:45:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 21:51:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-12 21:51:54]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dgozman, 2016-08-12 23:55:45]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2016-08-12 23:55:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
kozyatinskiy@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2226863003/#ps120001
(title: "wrong vector usage")

[commit-bot: I haz the power, 2016-08-12 23:56:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-13 00:00:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-13 00:00:25]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[caseq, 2016-08-13 00:01:08]

caseq@chromium.org changed reviewers:
+ caseq@chromium.org

[caseq, 2016-08-13 00:01:09]

https://codereview.chromium.org/2226863003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/inspector_protocol/String16.h (right):

https://codereview.chromium.org/2226863003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/inspector_protocol/String16.h:40: static char
buffer[50];
shouldn't we support multiple threads? ;-)

https://codereview.chromium.org/2226863003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/inspector_protocol/String16.h:47: static char
buffer[100];
ditto

https://codereview.chromium.org/2226863003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/inspector_protocol/String16.h:54: static char
buffer[100];
ditto

https://codereview.chromium.org/2226863003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/inspector_protocol/String16.h:61: static char
buffer[100];
ditto

[caseq, 2016-08-13 00:28:54]

https://codereview.chromium.org/2226863003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector/console/console-log-side-effects-expected.txt
(right):

https://codereview.chromium.org/2226863003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/console/console-log-side-effects-expected.txt:10:
CONSOLE MESSAGE: line 26: -4.242e-11
is this change intentional?

https://codereview.chromium.org/2226863003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/inspector_protocol/String16.cpp (right):

https://codereview.chromium.org/2226863003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/inspector_protocol/String16.cpp:67:
m_buffer.insert(m_buffer.end(), c);
m_buffer.push_back(c)

https://codereview.chromium.org/2226863003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/inspector_protocol/String16.cpp:70: void
String16Builder::append(char c)
I wonder if you really need the overload, wouldn't the one above be sufficient?

https://codereview.chromium.org/2226863003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/inspector_protocol/String16.h (right):

https://codereview.chromium.org/2226863003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/inspector_protocol/String16.h:35: return
isASCII(c) ? c <= ' ' && (c == ' ' || (c <= 0xD && c >= 0x9)) : false;
isASCII() && ....

https://codereview.chromium.org/2226863003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/inspector_protocol/String16.h:74: return 0;
consider returning NaN perhaps?

https://codereview.chromium.org/2226863003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/inspector_protocol/String16STL.h
(right):

https://codereview.chromium.org/2226863003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/inspector_protocol/String16STL.h:41: String16
substring(unsigned pos, unsigned len = 0xFFFFFFFF) const { return
String16(m_impl.substr(pos, len)); }
Shouldn't this be UINT_MAX?

https://codereview.chromium.org/2226863003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/inspector_protocol/String16STL.h:43: size_t
reverseFind(const String16& str, unsigned start = 0xFFFFFFFF) const { return
m_impl.rfind(str.m_impl, start); }
ditto.

https://codereview.chromium.org/2226863003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/inspector_protocol/String16WTF.h
(right):

https://codereview.chromium.org/2226863003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/inspector_protocol/String16WTF.h:32: for
(size_t i = 0; i < length; ++i)
consider using memcpy() perhaps?

https://codereview.chromium.org/2226863003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/v8_inspector/V8StringUtil.cpp (right):

https://codereview.chromium.org/2226863003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8StringUtil.cpp:85: String16
specials("[](){}+-*.,?\\^$|");
unused now?

[dgozman, 2016-08-13 04:49:59]

https://codereview.chromium.org/2226863003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/inspector_protocol/String16.h (right):

https://codereview.chromium.org/2226863003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/inspector_protocol/String16.h:40: static char
buffer[50];
On 2016/08/13 00:01:09, caseq wrote:
> shouldn't we support multiple threads? ;-)

Done.

https://codereview.chromium.org/2226863003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector/console/console-log-side-effects-expected.txt
(right):

https://codereview.chromium.org/2226863003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/console/console-log-side-effects-expected.txt:10:
CONSOLE MESSAGE: line 26: -4.242e-11
On 2016/08/13 00:28:53, caseq wrote:
> is this change intentional?

Yes.

https://codereview.chromium.org/2226863003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/inspector_protocol/String16.cpp (right):

https://codereview.chromium.org/2226863003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/inspector_protocol/String16.cpp:67:
m_buffer.insert(m_buffer.end(), c);
On 2016/08/13 00:28:53, caseq wrote:
> m_buffer.push_back(c)

Done.

https://codereview.chromium.org/2226863003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/inspector_protocol/String16.cpp:70: void
String16Builder::append(char c)
On 2016/08/13 00:28:53, caseq wrote:
> I wonder if you really need the overload, wouldn't the one above be
sufficient?

Just to be sure. WTF has one.

https://codereview.chromium.org/2226863003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/inspector_protocol/String16.h (right):

https://codereview.chromium.org/2226863003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/inspector_protocol/String16.h:35: return
isASCII(c) ? c <= ' ' && (c == ' ' || (c <= 0xD && c >= 0x9)) : false;
On 2016/08/13 00:28:53, caseq wrote:
> isASCII() && ....

Done.

https://codereview.chromium.org/2226863003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/inspector_protocol/String16.h:74: return 0;
On 2016/08/13 00:28:54, caseq wrote:
> consider returning NaN perhaps?

I don't think it matters. In fact, NaN could cause problems at some places not
expecting it.

https://codereview.chromium.org/2226863003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/inspector_protocol/String16STL.h
(right):

https://codereview.chromium.org/2226863003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/inspector_protocol/String16STL.h:41: String16
substring(unsigned pos, unsigned len = 0xFFFFFFFF) const { return
String16(m_impl.substr(pos, len)); }
On 2016/08/13 00:28:54, caseq wrote:
> Shouldn't this be UINT_MAX?

Done.

https://codereview.chromium.org/2226863003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/inspector_protocol/String16WTF.h
(right):

https://codereview.chromium.org/2226863003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/inspector_protocol/String16WTF.h:32: for
(size_t i = 0; i < length; ++i)
On 2016/08/13 00:28:54, caseq wrote:
> consider using memcpy() perhaps?

They have different sizes.

https://codereview.chromium.org/2226863003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/v8_inspector/V8StringUtil.cpp (right):

https://codereview.chromium.org/2226863003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/v8_inspector/V8StringUtil.cpp:85: String16
specials("[](){}+-*.,?\\^$|");
On 2016/08/13 00:28:54, caseq wrote:
> unused now?

Done.

[dgozman, 2016-08-13 04:50:07]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2016-08-13 04:50:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
kozyatinskiy@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2226863003/#ps160001
(title: "rebased")

[commit-bot: I haz the power, 2016-08-13 04:50:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-13 06:27:11]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-08-13 06:28:45]

Description was changed from

==========
[DevTools] Reduce API surface of String16.

Also, do not mention WTF::String anywhere except for String16WTF.
Drive-by: improved performance of String16Builder.

BUG=580337
==========

to

==========
[DevTools] Reduce API surface of String16.

Also, do not mention WTF::String anywhere except for String16WTF.
Drive-by: improved performance of String16Builder.

BUG=580337

Committed: https://crrev.com/16523d99c644c8513ad586eaca935fa2bea877b1
Cr-Commit-Position: refs/heads/master@{#411882}
==========

[commit-bot: I haz the power, 2016-08-13 06:28:46]

Patchset 9 (id:??) landed as
https://crrev.com/16523d99c644c8513ad586eaca935fa2bea877b1
Cr-Commit-Position: refs/heads/master@{#411882}

[esprehn, 2016-08-15 20:46:51]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2016-08-15 20:46:52]

I might suggest just removing the operator+ entirely btw, it causes bad copies
everywhere it's used. Instead I'd encourage StringBuilder or some function like
concat() that has versions with 2, 3, 4, 5 arguments or something?

https://codereview.chromium.org/2226863003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/inspector_protocol/String16WTF.h
(right):

https://codereview.chromium.org/2226863003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/inspector_protocol/String16WTF.h:67: inline
String16 operator+(const String16& a, const char* b) { return String16(a.impl()
+ b); }
Btw this is very slow in WTF, you're forcing tons of intermediate string copies
since this doesn't use the template magic of StringConcatenate and doesn't use
StringBuilder.

The STL one is also going to be slow since you're not reusing the same string so
the overallocation inside std::string is defeated.

ex.

a + b + c + d

makes 2 unneeded copies.

[dgozman, 2016-08-15 21:05:14]

https://codereview.chromium.org/2226863003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/inspector_protocol/String16WTF.h
(right):

https://codereview.chromium.org/2226863003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/inspector_protocol/String16WTF.h:67: inline
String16 operator+(const String16& a, const char* b) { return String16(a.impl()
+ b); }
On 2016/08/15 20:46:52, esprehn wrote:
> Btw this is very slow in WTF, you're forcing tons of intermediate string
copies
> since this doesn't use the template magic of StringConcatenate and doesn't use
> StringBuilder.
> 
> The STL one is also going to be slow since you're not reusing the same string
so
> the overallocation inside std::string is defeated.
> 
> ex.
> 
> a + b + c + d
> 
> makes 2 unneeded copies.

Thanks for feedback! I'm trying to use WTF::String directly in next patches, so
it should be good in that case. It could be possible to remove operator
entirely, I'll look into it.

[caseq, 2016-08-15 21:06:42]

On 2016/08/15 20:46:52, esprehn wrote:
> The STL one is also going to be slow since you're not reusing the same string
so
> the overallocation inside std::string is defeated.
> 
> ex.
> 
> a + b + c + d
> 
> makes 2 unneeded copies.

not trying to defend string::operator+ in anyway, but I think STL is doing fine
here -- (a + b) is going to produce an rvalue ref with the following +'s being
effectively appends(), and string capacity is preserved by the moving
constructor.

[esprehn, 2016-08-16 02:25:20]

On 2016/08/15 at 21:06:42, caseq wrote:
> On 2016/08/15 20:46:52, esprehn wrote:
> > The STL one is also going to be slow since you're not reusing the same
string so
> > the overallocation inside std::string is defeated.
> > 
> > ex.
> > 
> > a + b + c + d
> > 
> > makes 2 unneeded copies.
> 
> not trying to defend string::operator+ in anyway, but I think STL is doing
fine here -- (a + b) is going to produce an rvalue ref with the following +'s
being effectively appends(), and string capacity is preserved by the moving
constructor.

Are you sure? String16's operator+ returns another String16, so doing:

a + b + c is a + b, which does: String16(a.impl() + b.impl()); which is
String16(std::basic_string<UChar> + std::basic_string<UChar>) which then calls
String16(const std::basic_string<UChar>& impl) : m_impl(impl) which creates a
copy when assigning the m_impl object. We then do temporary + c which does the
same thing and you get the final value. I don't think a const
std::basic_string<UChar>& turns into a move() into m_impl, I think that would
mutate the referenced object.

[caseq, 2016-08-16 23:50:52]

On 2016/08/16 02:25:20, esprehn wrote:
> On 2016/08/15 at 21:06:42, caseq wrote:
> > On 2016/08/15 20:46:52, esprehn wrote:
> > > The STL one is also going to be slow since you're not reusing the same
> string so
> > > the overallocation inside std::string is defeated.
> > > 
> > > ex.
> > > 
> > > a + b + c + d
> > > 
> > > makes 2 unneeded copies.
> > 
> > not trying to defend string::operator+ in anyway, but I think STL is doing
> fine here -- (a + b) is going to produce an rvalue ref with the following +'s
> being effectively appends(), and string capacity is preserved by the moving
> constructor.
> 
> Are you sure? String16's operator+ returns another String16, so doing:
> 
> a + b + c is a + b, which does: String16(a.impl() + b.impl()); which is
> String16(std::basic_string<UChar> + std::basic_string<UChar>) which then calls
> String16(const std::basic_string<UChar>& impl) : m_impl(impl) which creates a
> copy when assigning the m_impl object. We then do temporary + c which does the
> same thing and you get the final value. I don't think a const
> std::basic_string<UChar>& turns into a move() into m_impl, I think that would
> mutate the referenced object.

Apologies, I thought we're talking of actual STL strings as opposed to
String16's that wrap std::string. The latter will have extra allocs now because
we don't seem to have operator+ overloads for rvalue refs (so would be easy to
fix if we need to), but my understanding is that Dmitry is going to switch
generator to directly use underlying string types, either eliminating String16
altogether or leaving it only as a collection of static utility methods -- so
only bare strings should matter long term.