[webkit-reviews] review granted: [Bug 221593] [JSC] Make JSON.parse faster by using table for fast string parsing : [Attachment 419674] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 221593: [JSC] Make JSON.parse faster by using table for fast string parsing
https://bugs.webkit.org/show_bug.cgi?id=221593

Attachment 419674: Patch

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




--- Comment #2 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 419674
  --> https://bugs.webkit.org/attachment.cgi?id=419674
Patch

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

> Source/JavaScriptCore/runtime/LiteralParser.cpp:437
> +enum StringCharacter : uint8_t { Slow, Fast, };
> +static constexpr const StringCharacter
safeStringLatin1CharactersInStrictJSON[256] = {

What does slow & fast mean? StringCharacter is also a very generic term to use
here.
Maybe renaming this to safeLatin1CharacterInStrictJSON and using boolean would
be better.
If we wanted to use enum class maybe something like this will be more
appropriate:
enum class JSONLiteralCharacterType : uint8_t { Safe, NotSafeInStrictMode };

> Source/JavaScriptCore/runtime/LiteralParser.cpp:839
> +	   return (c >= ' ' && c != '\\' && c != terminator) || (c == '\t');

Isn't this really just safeStringLatin1CharactersInStrictJSON[c] ==
StringCharacter::Fast || c == '\t'?

> Source/JavaScriptCore/runtime/LiteralParser.cpp:850
> +	   return (c >= ' ' && isLatin1(c) && c != '\\' && c != terminator) ||
(c == '\t');

Ditto.