Remove StringBuilder::appendLiteral.

Remove StringBuilder::appendLiteral.

We can just rely on the append(StringView) version instead. This does
mean we have to take 2 branches in some cases, but it should be
very minimal and since the code is inline the compiler can also
likely eliminate it.

This was originally committed as:
https://crrev.com/04157e8b89881d033f9eeca4466d9dd0c4e9aaea
but was reverted due to a bad assert that landed in a dependent
patch.

BUG=615174
Committed: https://crrev.com/41584c4ff6bc7a0556ffbbb3eaa90d46f7ddd015
Cr-Commit-Position: refs/heads/master@{#396996}

[esprehn, 2016-05-27 21:04:16]

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

[commit-bot: I haz the power, 2016-05-27 21:04:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2017053003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2017053003/1

[esprehn, 2016-05-27 21:25:45]

esprehn@chromium.org changed reviewers:
+ haraken@chromium.org, yutak@chromium.org

[haraken, 2016-05-27 21:41:42]

Did you confirm that this doesn't regress performance? I'm not sure if compilers
(esp. MSVC) are smart enough to calculate the string length at compile time.

[esprehn, 2016-05-27 22:01:42]

On 2016/05/27 at 21:41:42, haraken wrote:
> Did you confirm that this doesn't regress performance? I'm not sure if
compilers (esp. MSVC) are smart enough to calculate the string length at compile
time.

This is what base::StringPiece depends on, and it's what I've been doing
elsewhere in the codebase as well.

http://stackoverflow.com/questions/36389936/strlen-compile-time-optimization 

claims that MSVC 2015 definitely handles strlen optimization in many cases. This
seems easily reversible if the bots yell at us, or if we start seeing it in
profiles. That would also indicate that we need to do something about base's
usage of strlen.

[Jeffrey Yasskin, 2016-05-27 22:06:08]

On 2016/05/27 22:01:42, esprehn wrote:
> On 2016/05/27 at 21:41:42, haraken wrote:
> > Did you confirm that this doesn't regress performance? I'm not sure if
> compilers (esp. MSVC) are smart enough to calculate the string length at
compile
> time.
> 
> This is what base::StringPiece depends on, and it's what I've been doing
> elsewhere in the codebase as well.
> 
> http://stackoverflow.com/questions/36389936/strlen-compile-time-optimization 
> 
> claims that MSVC 2015 definitely handles strlen optimization in many cases.
This
> seems easily reversible if the bots yell at us, or if we start seeing it in
> profiles. That would also indicate that we need to do something about base's
> usage of strlen.

Clang and gcc definitely do this optimization. I haven't personally checked
MSVC, but if it doesn't, it'd be good to file a bug with them so they can fix
it.

[esprehn, 2016-05-27 22:20:54]

On 2016/05/27 at 22:06:08, jyasskin wrote:
> On 2016/05/27 22:01:42, esprehn wrote:
> > On 2016/05/27 at 21:41:42, haraken wrote:
> > > Did you confirm that this doesn't regress performance? I'm not sure if
> > compilers (esp. MSVC) are smart enough to calculate the string length at
compile
> > time.
> > 
> > This is what base::StringPiece depends on, and it's what I've been doing
> > elsewhere in the codebase as well.
> > 
> > http://stackoverflow.com/questions/36389936/strlen-compile-time-optimization

> > 
> > claims that MSVC 2015 definitely handles strlen optimization in many cases.
This
> > seems easily reversible if the bots yell at us, or if we start seeing it in
> > profiles. That would also indicate that we need to do something about base's
> > usage of strlen.
> 
> Clang and gcc definitely do this optimization. I haven't personally checked
MSVC, but if it doesn't, it'd be good to file a bug with them so they can fix
it.

I'd like to land this and see what the perf bots tell us, are you okay with that
haraken@? :)

[esprehn, 2016-05-27 22:22:37]

On 2016/05/27 at 22:20:54, esprehn wrote:
> On 2016/05/27 at 22:06:08, jyasskin wrote:
> > ...
> > 
> > Clang and gcc definitely do this optimization. I haven't personally checked
MSVC, but if it doesn't, it'd be good to file a bug with them so they can fix
it.
> 
> I'd like to land this and see what the perf bots tell us, are you okay with
that haraken@? :)

Note that we've already landed two patches that do this:

https://codereview.chromium.org/1845363003
https://codereview.chromium.org/1844223002

[haraken, 2016-05-27 22:27:48]

On 2016/05/27 22:22:37, esprehn wrote:
> On 2016/05/27 at 22:20:54, esprehn wrote:
> > On 2016/05/27 at 22:06:08, jyasskin wrote:
> > > ...
> > > 
> > > Clang and gcc definitely do this optimization. I haven't personally
checked
> MSVC, but if it doesn't, it'd be good to file a bug with them so they can fix
> it.
> > 
> > I'd like to land this and see what the perf bots tell us, are you okay with
> that haraken@? :)
> 
> Note that we've already landed two patches that do this:
> 
> https://codereview.chromium.org/1845363003
> https://codereview.chromium.org/1844223002

LGTM :)

[commit-bot: I haz the power, 2016-05-27 23:04:27]

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

[commit-bot: I haz the power, 2016-05-27 23:04:29]

Dry run: This issue passed the CQ dry run.

[esprehn, 2016-05-27 23:37:06]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2016-05-27 23:37:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2017053003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2017053003/1

[commit-bot: I haz the power, 2016-05-27 23:43:55]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-05-27 23:45:02]

Description was changed from

==========
Remove StringBuilder::appendLiteral.

We can just rely on the append(StringView) version instead. This does
mean we have to take 2 branches in some cases, but it should be
very minimal and since the code is inline the compiler can also
likely eliminate it.

BUG=615174
==========

to

==========
Remove StringBuilder::appendLiteral.

We can just rely on the append(StringView) version instead. This does
mean we have to take 2 branches in some cases, but it should be
very minimal and since the code is inline the compiler can also
likely eliminate it.

BUG=615174

Committed: https://crrev.com/04157e8b89881d033f9eeca4466d9dd0c4e9aaea
Cr-Commit-Position: refs/heads/master@{#396601}
==========

[commit-bot: I haz the power, 2016-05-27 23:45:04]

Patchset 1 (id:??) landed as
https://crrev.com/04157e8b89881d033f9eeca4466d9dd0c4e9aaea
Cr-Commit-Position: refs/heads/master@{#396601}

[sof, 2016-05-28 11:48:16]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/2017303002/ by sigbjornf@opera.com.

The reason for reverting is: Will unfortunately have to back out this one so
that the revert https://codereview.chromium.org/2025503002/ will cleanly apply.

(Don't want a tree with this many ASan failures over the long weekend.).

[esprehn, 2016-05-31 22:15:47]

Description was changed from

==========
Remove StringBuilder::appendLiteral.

We can just rely on the append(StringView) version instead. This does
mean we have to take 2 branches in some cases, but it should be
very minimal and since the code is inline the compiler can also
likely eliminate it.

BUG=615174

Committed: https://crrev.com/04157e8b89881d033f9eeca4466d9dd0c4e9aaea
Cr-Commit-Position: refs/heads/master@{#396601}
==========

to

==========
Remove StringBuilder::appendLiteral.

We can just rely on the append(StringView) version instead. This does
mean we have to take 2 branches in some cases, but it should be
very minimal and since the code is inline the compiler can also
likely eliminate it.

This was originally committed as:
https://crrev.com/04157e8b89881d033f9eeca4466d9dd0c4e9aaea
but was reverted due to a bad assert that landed in a dependent
patch.

BUG=615174
==========

[esprehn, 2016-05-31 22:15:53]

The CQ bit was checked by esprehn@chromium.org

[esprehn, 2016-05-31 22:15:54]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2017053003/#ps20001
(title: "rebase.")

[commit-bot: I haz the power, 2016-05-31 22:16:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2017053003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2017053003/20001

[commit-bot: I haz the power, 2016-06-01 00:48:16]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-01 00:49:31]

Description was changed from

==========
Remove StringBuilder::appendLiteral.

We can just rely on the append(StringView) version instead. This does
mean we have to take 2 branches in some cases, but it should be
very minimal and since the code is inline the compiler can also
likely eliminate it.

This was originally committed as:
https://crrev.com/04157e8b89881d033f9eeca4466d9dd0c4e9aaea
but was reverted due to a bad assert that landed in a dependent
patch.

BUG=615174
==========

to

==========
Remove StringBuilder::appendLiteral.

We can just rely on the append(StringView) version instead. This does
mean we have to take 2 branches in some cases, but it should be
very minimal and since the code is inline the compiler can also
likely eliminate it.

This was originally committed as:
https://crrev.com/04157e8b89881d033f9eeca4466d9dd0c4e9aaea
but was reverted due to a bad assert that landed in a dependent
patch.

BUG=615174

Committed: https://crrev.com/41584c4ff6bc7a0556ffbbb3eaa90d46f7ddd015
Cr-Commit-Position: refs/heads/master@{#396996}
==========

[commit-bot: I haz the power, 2016-06-01 00:49:34]

Patchset 2 (id:??) landed as
https://crrev.com/41584c4ff6bc7a0556ffbbb3eaa90d46f7ddd015
Cr-Commit-Position: refs/heads/master@{#396996}