Bug 180966 – [YARR] Yarr should return ErrorCode instead of error messages (const char*)

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[YARR] Yarr should return ErrorCode instead of error messages (const char*)

[Yusuke Suzuki]

Created attachment 329742 [details]
Patch

[EWS Watchlist]

Attachment 329742 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:36:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:37:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:38:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:39:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:40:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:41:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:42:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:43:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:44:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:45:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:46:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:47:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:48:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:49:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:50:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:51:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:52:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:53:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:54:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:55:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 20 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 329752 [details]
Patch

[EWS Watchlist]

Attachment 329752 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:36:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:37:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:38:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:39:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:40:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:41:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:42:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:43:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:44:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:45:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:46:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:47:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:48:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:49:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:50:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:51:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:52:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:53:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:54:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:55:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 20 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 329754 [details]
Patch

[EWS Watchlist]

Attachment 329754 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:36:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:37:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:38:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:39:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:40:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:41:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:42:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:43:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:44:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:45:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:46:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:47:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:48:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:49:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:50:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:51:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:52:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:53:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:54:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:55:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 20 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 329755 [details]
Patch

[EWS Watchlist]

Attachment 329755 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:36:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:37:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:38:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:39:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:40:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:41:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:42:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:43:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:44:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:45:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:46:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:47:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:48:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:49:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:50:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:51:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:52:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:53:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:54:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/yarr/YarrErrorCode.cpp:55:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 20 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 329755 [details]
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?

[Yusuke Suzuki]

Comment on attachment 329755 [details]
Patch

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

Thanks!

>> 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."

Fixed.

>> 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".

Fixed.

>> 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.

Nice. I'll do this change in this patch.

>> Source/JavaScriptCore/yarr/RegularExpression.cpp:78
>> +    JSC::Yarr::ErrorCode m_constructionError { Yarr::ErrorCode::NoError };
> 
> Ditto.  I suggest renaming to m_constructionErrorCode.

Fixed.

>> Source/JavaScriptCore/yarr/YarrParser.h:1105
>> +    ErrorCode m_err { ErrorCode::NoError };
> 
> I suggest renaming m_err to m_errCode to match the new type.

Fixed. I've changed this to 'm_errorCode'.

>> 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.

Sounds nice. Fixed.

>> 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?

Yeah, we can just use it. Fixed.

[Yusuke Suzuki]

Committed r226128: <https://trac.webkit.org/changeset/226128>

[Radar WebKit Bug Importer]

<rdar://problem/36136121>

[Yusuke Suzuki]

Committed r226129: <https://trac.webkit.org/changeset/226129>