[webkit-reviews] review granted: [Bug 178174] Add support in named capture group identifiers for direct surrogate pairs : [Attachment 394700] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Shvayka
<shvaikalesh at gmail.com>'s request for review:
Bug 178174: Add support in named capture group identifiers for direct surrogate
pairs
https://bugs.webkit.org/show_bug.cgi?id=178174

Attachment 394700: Patch

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




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

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

This looks fine to me. Michael may also want to review. The key is really test
coverage including performance test results, not necessarily exactly how we
write the code

> Source/JavaScriptCore/yarr/YarrParser.h:493
> +	       int escapeChar = tryConsumeUnicodeEscape();

Normally we don’t use abbreviations like "char" in WebKit code, preferring
words like "character". So this local variable would be either named "escape"
or "escapeCharacter" or "escaped" or "escapedCharacter" or "codePoint".

> Source/JavaScriptCore/yarr/YarrParser.h:-1044
> -	   if (u == -1)
> -	       return -1;

What’s the rationale for removing this? I don’t think it’s great to rely on
what U16_IS_LEAD will return when passed something that is not a 16-bit code
unit. So I would have left it in.

> Source/JavaScriptCore/yarr/YarrParser.h:1031
> +	       int escapeChar = tryConsumeUnicodeEscape();

Same comment about variable naming here. Names like "ch" and "u" are definitely
frowned upon in our general coding style, but in a way "escapeChar" is half-way
there. Our underlying concept is that people are better at reading words than
abbreviations.