Bug 180966

Summary:

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

Product:

WebKit

Reporter:

Yusuke Suzuki <ysuzuki>

Component:

New Bugs

Assignee:

Yusuke Suzuki <ysuzuki>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ews-watchlist, joepeck, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

180970

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	mark.lam: review+

Yusuke Suzuki

Reported 2017-12-18 23:41:34 PST

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

Attachments
Patch (50.90 KB, patch) 2017-12-19 00:26 PST, Yusuke Suzuki	no flags	Details Formatted Diff Diff
Patch (56.00 KB, patch) 2017-12-19 03:42 PST, Yusuke Suzuki	no flags	Details Formatted Diff Diff
Patch (57.63 KB, patch) 2017-12-19 04:19 PST, Yusuke Suzuki	no flags	Details Formatted Diff Diff
Patch (57.63 KB, patch) 2017-12-19 04:30 PST, Yusuke Suzuki	mark.lam: review+	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Yusuke Suzuki

Comment 1 2017-12-19 00:26:15 PST

Created attachment 329742 [details] Patch

EWS Watchlist

Comment 2 2017-12-19 00:28:53 PST

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

Comment 3 2017-12-19 03:42:24 PST

Created attachment 329752 [details] Patch

EWS Watchlist

Comment 4 2017-12-19 03:43:55 PST

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

Comment 5 2017-12-19 04:19:09 PST

Created attachment 329754 [details] Patch

EWS Watchlist

Comment 6 2017-12-19 04:20:46 PST

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

Comment 7 2017-12-19 04:30:43 PST

Created attachment 329755 [details] Patch

EWS Watchlist

Comment 8 2017-12-19 04:32:14 PST

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 9 2017-12-19 09:41:35 PST

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 10 2017-12-19 11:15:26 PST

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

Comment 11 2017-12-19 11:16:27 PST

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

Radar WebKit Bug Importer

Comment 12 2017-12-19 11:17:37 PST

<rdar://problem/36136121>

Yusuke Suzuki

Comment 13 2017-12-19 11:27:40 PST

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

Note You need to log in before you can comment on or make changes to this bug.