[webkit-reviews] review granted: [Bug 172069] Implement RegExp Unicode property escapes : [Attachment 323051] Main Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Oct 6 15:53:39 PDT 2017
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 &
More information about the webkit-reviews
mailing list