[webkit-reviews] review granted: [Bug 238162] String literals with _str suffix should not need to do strlen() at runtime : [Attachment 455332] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 238162: String literals with _str suffix should not need to do strlen() at
runtime
https://bugs.webkit.org/show_bug.cgi?id=238162

Attachment 455332: Patch

https://bugs.webkit.org/attachment.cgi?id=455332&action=review




--- Comment #5 from Darin Adler <darin at apple.com> ---
Comment on attachment 455332
  --> https://bugs.webkit.org/attachment.cgi?id=455332
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=455332&action=review

So unclear to me when to use _s vs. _str — please write down a guideline for
future use somewhere. Maybe we’ll even want to rename one or both so it’s more
obvious when one is right vs. the other.

If ""_s was smaller code than ""_str then you’d want to decide based on how
likely a code path was perhaps? I personally don’t like having a choice when
it’s not obvious which is better, and there are so often choices where you
could use "", ""_s, or ""_str.

For example, does a string literal argument to makeString compile into a code
sequence that involves call to strlen? Or does that get constant-folded? What
about if you use ""_s or ""_str instead of just ""?

> Source/WTF/ChangeLog:9
> +	   it could trigger calls to strlen() at run time to figure out the
length of the string. This was unforunate

Typo in unfortunate

> Source/WTF/ChangeLog:14
> +	   This patch also adopts the ""_str operator in more places so they
can leverage the optimization.

Is this a measurable speedup? Is it a measurable code size increase or
decrease?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2538
> +		   return $useAtomString ? "AtomString(${defaultValue},
AtomString::ConstructFromLiteral)" : "${defaultValue}_str";

Looks like you need to rebase the bindings test results because of these
changes to CodeGeneratorJS.pm.