[webkit-reviews] review granted: [Bug 172069] Implement RegExp Unicode property escapes : [Attachment 323051] Main Patch

[bugzilla-daemon at webkit.org]

JF Bastien <jfbastien at apple.com> has granted Michael Saboff
<msaboff at apple.com>'s request for review:
Bug 172069: Implement RegExp Unicode property escapes
https://bugs.webkit.org/show_bug.cgi?id=172069

Attachment 323051: Main Patch

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




--- Comment #9 from JF Bastien <jfbastien at apple.com> ---
Comment on attachment 323051
  --> https://bugs.webkit.org/attachment.cgi?id=323051
Main Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:22
> +	   names to the function index.  Using these hashing tables, we can
lookup up a property

up up up up

> Source/JavaScriptCore/Scripts/generateYarrUnicodePropertyTables.py:1
> +#! /usr/bin/python

#!/usr/bin/env python

> Source/JavaScriptCore/Scripts/generateYarrUnicodePropertyTables.py:62
> +// DO NO EDIT! - This file was generated by
generateYarrUnicodePropertyTables.py

""" + __file__ + """

> Source/JavaScriptCore/Scripts/generateYarrUnicodePropertyTables.py:117
> +	   exit(1)

This is all a lot of error handling for a python script! Such dedication, I'd
just `with open(...) as ...:` and call it a day. :)

> Source/JavaScriptCore/Scripts/hasher.py:1
> +#! /usr/bin/python

Ditto on shebang.

> Source/JavaScriptCore/Scripts/hasher.py:27
> +# Boston, MA 02110-1301, USA.

Isn't this all newly translated code?

> Source/JavaScriptCore/yarr/YarrInterpreter.cpp:307
> +	       size_t high = matches.size() - 1;

Is it ever empty?

> Source/JavaScriptCore/yarr/YarrInterpreter.cpp:336
> +	       size_t high = ranges.size() - 1;

Ditto.

> Source/JavaScriptCore/yarr/YarrInterpreter.cpp:354
> +	   const size_t thresholdForBinarySearch = 6;

I guess it's never empty because of this!

Did you choose 6 out of some scientific measurement? Or pulled the number from
a Magical Place?

> Source/JavaScriptCore/yarr/YarrParser.h:458
> +		       // m_err should be set
tryConsumeUnicodePropertyExpression()

I don't get this comment. "set by" ?

> Source/JavaScriptCore/yarr/YarrPattern.cpp:98
> +		   addSortedMatchOrRange(min, max + 1);

Can this ever overflow max? Ditto below for the arithmetic.

> Source/JavaScriptCore/yarr/YarrPattern.cpp:418
>      void atomBuiltInCharacterClass(BuiltInCharacterClassID classID, bool
invert)

I find calls to this hard to read because of the boolean param. Could you make
it an enum class CharacterClassInvert { Do, Dont }; or something similar?

> Source/JavaScriptCore/yarr/YarrPattern.cpp:1389
> +

Extra

> Source/JavaScriptCore/yarr/YarrPattern.h:50
> +    }

You could just set these values above, and then rely on the compiler
auto-generating the default ctor properly.

> Source/JavaScriptCore/yarr/YarrUnicodeProperties.cpp:52
> +    ALWAYS_INLINE int entry(WTF::String key) const

const WTF::String &   ?

> Source/JavaScriptCore/yarr/YarrUnicodeProperties.cpp:75
> +std::optional<BuiltInCharacterClassID> unicodeMatchPropertyValue(WTF::String
unicodePropertyName, WTF::String unicodePropertyValue)

const WTF::String &

> Source/JavaScriptCore/yarr/YarrUnicodeProperties.cpp:92
> +std::optional<BuiltInCharacterClassID> unicodeMatchProperty(WTF::String
unicodePropertyValue)

const WTF::String &