Summary: | YARR: Coalesce constructed character classes | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Saboff <msaboff> | ||||
Component: | JavaScriptCore | Assignee: | Michael Saboff <msaboff> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | ews-watchlist, fpizlo, jfbastien, keith_miller, mark.lam, saam, webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | Other | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 179230 | ||||||
Attachments: |
|
Description
Michael Saboff
2017-12-07 11:34:40 PST
Created attachment 328716 [details]
Patch
Attachment 328716 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/yarr/YarrPattern.cpp:406: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
Total errors found: 1 in 5 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 328716 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328716&action=review I'm not an expert in this code, but looks fine overall. Minor comments. > Source/JavaScriptCore/yarr/YarrPattern.cpp:286 > + if (pos + index > 0 && matches[pos + index - 1] == ch - 1) { pos and index are both unsigned, so this is just checking that it's non-zero? Or was the intent to capture wraparound as well? > Source/JavaScriptCore/yarr/YarrPattern.cpp:358 > + // each iteration of the loop we will either remove something from the list, or break the loop. Break the loop, or just break out of it? > Source/JavaScriptCore/yarr/YarrPattern.cpp:407 > + && m_rangesUnicode[0].begin == 0x80 && m_rangesUnicode[0].end == 0x10ffff) I don't get the Unicode range comparison. That's the general non-ASCII range, can the user specify invalid codepoint ranges? Or put another way, when it this range *not* the Unicode range? Comment on attachment 328716 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328716&action=review >> Source/JavaScriptCore/yarr/YarrPattern.cpp:286 >> + if (pos + index > 0 && matches[pos + index - 1] == ch - 1) { > > pos and index are both unsigned, so this is just checking that it's non-zero? Or was the intent to capture wraparound as well? Just checking that it's non-zero. Due to the range of character values (0..0x10ffff), we can't get close to wrapping around even if there was one character per range. >> Source/JavaScriptCore/yarr/YarrPattern.cpp:358 >> + // each iteration of the loop we will either remove something from the list, or break the loop. > > Break the loop, or just break out of it? Break *out of* the loop. >> Source/JavaScriptCore/yarr/YarrPattern.cpp:407 >> + && m_rangesUnicode[0].begin == 0x80 && m_rangesUnicode[0].end == 0x10ffff) > > I don't get the Unicode range comparison. That's the general non-ASCII range, can the user specify invalid codepoint ranges? > > Or put another way, when it this range *not* the Unicode range? This checks that this character class matches every possible character. Committed r225683: <https://trac.webkit.org/changeset/225683> |