Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1582813003:
Remove redundant emptiness check from genericParseNumber (Closed)
|
Created:
4 years, 11 months ago by fs Modified:
4 years, 11 months ago Reviewers:
pdr. CC:
blink-reviews, chromium-reviews, krit, f(malita), gyuyoung2, kouhei+svg_chromium.org, pdr+svgwatchlist_chromium.org, rwlbuis, Stephen Chennney Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove redundant emptiness check from genericParseNumber
When the 'start == ptr' condition was reached we could be sure that we
had already consumed at least one character - one of '.' or '0'-'9' or
whitespace (potentially also '+'/'-' as a prefix) - and that even holds
true moving the definition of |start| after consuming any leading
whitespace. The reason for this is the up-front check for any character
in the set '0'-'9' or '.'.
Remove the redundant checks - replacing them with an assert - while
moving the definition of |start| so that it doesn't point before any
leading whitespace.
BUG=231612
Committed: https://crrev.com/ab685d36561909a7ef373183f37fc3d6fc21bb83
Cr-Commit-Position: refs/heads/master@{#369292}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Re-use/name ptrStartIntPart. #Messages
Total messages: 19 (7 generated)
fs@opera.com changed reviewers: + pdr@chromium.org
Codechange LGTM, just a question about making the assert easier to follow. https://codereview.chromium.org/1582813003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/svg/SVGParserUtilities.cpp (right): https://codereview.chromium.org/1582813003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/svg/SVGParserUtilities.cpp:99: ASSERT_UNUSED(digitsStart, digitsStart != ptr); Can we reuse/rename ptrStartIntPart and change this to a regular ol' assertion?
https://codereview.chromium.org/1582813003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/svg/SVGParserUtilities.cpp (right): https://codereview.chromium.org/1582813003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/svg/SVGParserUtilities.cpp:99: ASSERT_UNUSED(digitsStart, digitsStart != ptr); On 2016/01/13 at 18:58:11, pdr wrote: > Can we reuse/rename ptrStartIntPart and change this to a regular ol' assertion? Yepp. Like so.
On 2016/01/13 at 19:16:27, fs wrote: > https://codereview.chromium.org/1582813003/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/svg/SVGParserUtilities.cpp (right): > > https://codereview.chromium.org/1582813003/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/svg/SVGParserUtilities.cpp:99: ASSERT_UNUSED(digitsStart, digitsStart != ptr); > On 2016/01/13 at 18:58:11, pdr wrote: > > Can we reuse/rename ptrStartIntPart and change this to a regular ol' assertion? > > Yepp. Like so. So LGTM!
On 2016/01/13 at 19:23:25, pdr wrote: > On 2016/01/13 at 19:16:27, fs wrote: > > https://codereview.chromium.org/1582813003/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/svg/SVGParserUtilities.cpp (right): > > > > https://codereview.chromium.org/1582813003/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/svg/SVGParserUtilities.cpp:99: ASSERT_UNUSED(digitsStart, digitsStart != ptr); > > On 2016/01/13 at 18:58:11, pdr wrote: > > > Can we reuse/rename ptrStartIntPart and change this to a regular ol' assertion? > > > > Yepp. Like so. > > So LGTM! That must be better than "so so LGTM"! =D
The CQ bit was checked by fs@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1582813003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1582813003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by fs@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1582813003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1582813003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by pdr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1582813003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1582813003/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Remove redundant emptiness check from genericParseNumber When the 'start == ptr' condition was reached we could be sure that we had already consumed at least one character - one of '.' or '0'-'9' or whitespace (potentially also '+'/'-' as a prefix) - and that even holds true moving the definition of |start| after consuming any leading whitespace. The reason for this is the up-front check for any character in the set '0'-'9' or '.'. Remove the redundant checks - replacing them with an assert - while moving the definition of |start| so that it doesn't point before any leading whitespace. BUG=231612 ========== to ========== Remove redundant emptiness check from genericParseNumber When the 'start == ptr' condition was reached we could be sure that we had already consumed at least one character - one of '.' or '0'-'9' or whitespace (potentially also '+'/'-' as a prefix) - and that even holds true moving the definition of |start| after consuming any leading whitespace. The reason for this is the up-front check for any character in the set '0'-'9' or '.'. Remove the redundant checks - replacing them with an assert - while moving the definition of |start| so that it doesn't point before any leading whitespace. BUG=231612 Committed: https://crrev.com/ab685d36561909a7ef373183f37fc3d6fc21bb83 Cr-Commit-Position: refs/heads/master@{#369292} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ab685d36561909a7ef373183f37fc3d6fc21bb83 Cr-Commit-Position: refs/heads/master@{#369292} |
