[webkit-reviews] review granted: [Bug 180966] [YARR] Yarr should return ErrorCode instead of error messages (const char*) : [Attachment 329755] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 180966: [YARR] Yarr should return ErrorCode instead of error messages
(const char*)
https://bugs.webkit.org/show_bug.cgi?id=180966

Attachment 329755: Patch

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




--- Comment #9 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 329755
  --> https://bugs.webkit.org/attachment.cgi?id=329755
Patch

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

r=me with suggestions.

> Source/JavaScriptCore/ChangeLog:8
> +	   Currently, Yarr returns const char*` to point an error message. But
it is

I suggest replacing "to point an error message." with "for an error message
when needed."

> Source/JavaScriptCore/ChangeLog:9
> +	   easier to handle error status that Yarr returns an error code
instead of

I suggest replacing "that Yarr returns" with "if Yarr returns".

> Source/JavaScriptCore/runtime/RegExp.h:156
> +    Yarr::ErrorCode m_constructionError { Yarr::ErrorCode::NoError };

I suggest renaming m_constructionError to m_constructionErrorCode. 
"m_constructionError" can be ambiguous in that it can suggest that there is
definitely an error.  "m_constructionErrorCode " also matches the new type. 
I'm ok with doing this renaming in a separate patch if you prefer to keep this
patch cleaner.

> Source/JavaScriptCore/yarr/RegularExpression.cpp:78
> +    JSC::Yarr::ErrorCode m_constructionError { Yarr::ErrorCode::NoError };

Ditto.	I suggest renaming to m_constructionErrorCode.

> Source/JavaScriptCore/yarr/YarrParser.h:1105
> +    ErrorCode m_err { ErrorCode::NoError };

I suggest renaming m_err to m_errCode to match the new type.

> Source/JavaScriptCore/yarr/YarrPattern.cpp:1131
> +YarrPattern::YarrPattern(const String& pattern, RegExpFlags flags,
ErrorCode* error, void* stackLimit)

Since we're always expecting a return error code, I suggest making this
argument a ErrorCode& instead of a pointer.

> Source/WebCore/contentextensions/URLFilterParser.cpp:351
> +    if (JSC::Yarr::parse(patternParser, pattern, false, 0) ==
JSC::Yarr::ErrorCode::NoError)

nit: Why not continue to use the same !hasError() idiom used everywhere else?