Summary: | String new RegExp('\n').toString() returns is invalid RegularExpressionLiteral | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | barraclough, jchaffraix, msaboff, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Yusuke Suzuki
2011-11-04 10:55:38 PDT
Created attachment 113682 [details]
Patch
attached patch
reported issue and sent patch to V8 http://code.google.com/p/v8/issues/detail?id=1813 Comment on attachment 113682 [details]
Patch
Good catch & nice fix. But I think we may want to make one tweak to this patch before landing. Most regular expressions don't contain forwards slashes or newlines, but this patch will always produce a fresh copy of the string every time source is called. Currently the code has an early return for strings that don't contain a forwards slash. We could expand this to also check for newline characters, and early return if the string contains neither forward slashes or newlines. r- for the loss of this optimization, but r+ in principle, we should definitely take this fix & the code looks great otherwise. :-)
(In reply to comment #3) > (From update of attachment 113682 [details]) > Good catch & nice fix. But I think we may want to make one tweak to this patch before landing. Most regular expressions don't contain forwards slashes or newlines, but this patch will always produce a fresh copy of the string every time source is called. Currently the code has an early return for strings that don't contain a forwards slash. We could expand this to also check for newline characters, and early return if the string contains neither forward slashes or newlines. r- for the loss of this optimization, but r+ in principle, we should definitely take this fix & the code looks great otherwise. :-) Thanks for your review. You're right. I'll add early return to this patch. Created attachment 115232 [details]
Patch
attached patch v2. First, checking this pattern should be escaped, and if it is false return early
Comment on attachment 115232 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115232&action=review Looks OK. Needs a couple small changes. > Source/JavaScriptCore/runtime/RegExpObject.cpp:124 > + bool previousIsBackSlash = false; backslash is one word, not two, so this should be named previousCharacterWasBackslash > Source/JavaScriptCore/runtime/RegExpObject.cpp:145 > + if (ch == '\n' || ch == '\r' || (ch & ~1) == 0x2028) { This should use the inline isLineTerminator function instead of repeating its code. > Source/JavaScriptCore/runtime/RegExpObject.cpp:157 > + if (!shouldEscape) > + return jsString(exec, pattern); This is indented wrong. Should be indented four spaces. > Source/JavaScriptCore/runtime/RegExpObject.cpp:177 > + if (ch == '\n' || ch == '\r' || (ch & ~1) == 0x2028) { This should use the inline isLineTerminator function instead of repeating its code. (In reply to comment #6) Thank you for your review. > backslash is one word, not two, so this should be named previousCharacterWasBackslash Yes, I'll rename it. > > Source/JavaScriptCore/runtime/RegExpObject.cpp:145 > > + if (ch == '\n' || ch == '\r' || (ch & ~1) == 0x2028) { > This should use the inline isLineTerminator function instead of repeating its code. Right, I'll change it to using Lexer<UChar>::isLineTerminator. > This is indented wrong. Should be indented four spaces. Oops. This is mistake. I'll upload revised patch soon. Created attachment 115244 [details]
Patch
attached patch v3
Comment on attachment 115244 [details]
Patch
Looks great.
Comment on attachment 115244 [details] Patch Clearing flags on attachment: 115244 Committed r100469: <http://trac.webkit.org/changeset/100469> All reviewed patches have been landed. Closing bug. |