Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2380003003:
Update PRESUBMIT check for use counters to handle C++ formatting changes (Closed)
DescriptionUpdate PRESUBMIT check for use counters to handle C++ formatting changes
Long-term, we probably want to consider generating this somehow, rather
than using regex to try to parse stuff out of C++.
BUG=649969
Committed: https://crrev.com/c4c4814a47286093f5ea69a9ae5070d14dd48572
Cr-Commit-Position: refs/heads/master@{#422017}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Simplify regex and fix comment. #Messages
Total messages: 15 (5 generated)
dcheng@chromium.org changed reviewers: + qyearsley@chromium.org
I think this is the last known presubmit change that we can split out of the clang-format mega CL. I'll need to do another test run, but I think this should hopefully be the last of the issues.
thakis@chromium.org changed reviewers: + thakis@chromium.org
lgtm https://codereview.chromium.org/2380003003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/PRESUBMIT.py (right): https://codereview.chromium.org/2380003003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/PRESUBMIT.py:25: r'case CSSProperty\w*?\:\s*?\n?\s+?return (\d+);', why \s+ instead of \s* ? So `return\n43;` doesn't happen in practice? https://codereview.chromium.org/2380003003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/PRESUBMIT.py:27: # Looking for a line like "int maximumCSSSampleId() { return 452; }" update comment
The CQ bit was checked by dcheng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2380003003/#ps20001 (title: "Simplify regex and fix comment.")
https://codereview.chromium.org/2380003003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/PRESUBMIT.py (right): https://codereview.chromium.org/2380003003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/PRESUBMIT.py:25: r'case CSSProperty\w*?\:\s*?\n?\s+?return (\d+);', On 2016/09/30 00:41:18, Nico (mostly away until Oct 3) wrote: > why \s+ instead of \s* ? > > So `return\n43;` doesn't happen in practice? I think \s will match \n, since we're matching in multiline mode. I simplified it a bit overall by replacing \s*?\n?\s+ with just \s+? https://codereview.chromium.org/2380003003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/PRESUBMIT.py:27: # Looking for a line like "int maximumCSSSampleId() { return 452; }" On 2016/09/30 00:41:18, Nico (mostly away until Oct 3) wrote: > update comment Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2380003003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/PRESUBMIT.py (right): https://codereview.chromium.org/2380003003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/PRESUBMIT.py:25: r'case CSSProperty\w*?\:\s*?\n?\s+?return (\d+);', On 2016/09/30 01:56:38, dcheng wrote: > On 2016/09/30 00:41:18, Nico (mostly away until Oct 3) wrote: > > why \s+ instead of \s* ? > > > > So `return\n43;` doesn't happen in practice? > > I think \s will match \n, since we're matching in multiline mode. I simplified > it a bit overall by replacing \s*?\n?\s+ with just \s+? I mean isn't `\s+?` the same as `\s*` ?
On 2016/09/30 02:24:52, Nico (mostly away until Oct 3) wrote: > https://codereview.chromium.org/2380003003/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/frame/PRESUBMIT.py (right): > > https://codereview.chromium.org/2380003003/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/frame/PRESUBMIT.py:25: r'case > CSSProperty\w*?\:\s*?\n?\s+?return (\d+);', > On 2016/09/30 01:56:38, dcheng wrote: > > On 2016/09/30 00:41:18, Nico (mostly away until Oct 3) wrote: > > > why \s+ instead of \s* ? > > > > > > So `return\n43;` doesn't happen in practice? > > > > I think \s will match \n, since we're matching in multiline mode. I simplified > > it a bit overall by replacing \s*?\n?\s+ with just \s+? > > I mean isn't `\s+?` the same as `\s*` ? + is 1 or more, * is 0 or more. I don't think it'll happen, but in case someone somehow uses return123124 in an identifier, we don't want to match that.
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Update PRESUBMIT check for use counters to handle C++ formatting changes Long-term, we probably want to consider generating this somehow, rather than using regex to try to parse stuff out of C++. BUG=649969 ========== to ========== Update PRESUBMIT check for use counters to handle C++ formatting changes Long-term, we probably want to consider generating this somehow, rather than using regex to try to parse stuff out of C++. BUG=649969 Committed: https://crrev.com/c4c4814a47286093f5ea69a9ae5070d14dd48572 Cr-Commit-Position: refs/heads/master@{#422017} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/c4c4814a47286093f5ea69a9ae5070d14dd48572 Cr-Commit-Position: refs/heads/master@{#422017}
Message was sent while issue was closed.
On 2016/09/30 02:30:21, dcheng wrote: > On 2016/09/30 02:24:52, Nico (mostly away until Oct 3) wrote: > > > https://codereview.chromium.org/2380003003/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/frame/PRESUBMIT.py (right): > > > > > https://codereview.chromium.org/2380003003/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/frame/PRESUBMIT.py:25: r'case > > CSSProperty\w*?\:\s*?\n?\s+?return (\d+);', > > On 2016/09/30 01:56:38, dcheng wrote: > > > On 2016/09/30 00:41:18, Nico (mostly away until Oct 3) wrote: > > > > why \s+ instead of \s* ? > > > > > > > > So `return\n43;` doesn't happen in practice? > > > > > > I think \s will match \n, since we're matching in multiline mode. I > simplified > > > it a bit overall by replacing \s*?\n?\s+ with just \s+? > > > > I mean isn't `\s+?` the same as `\s*` ? > > + is 1 or more, * is 0 or more. I don't think it'll happen, but in case someone > somehow uses return123124 in an identifier, we don't want to match that. Well yeah, but if you put ? after \s+ it means "one or more, or not" which seems like the same as "zero or more"
Message was sent while issue was closed.
On 2016/09/30 13:30:15, Nico (mostly away until Oct 3) wrote: > On 2016/09/30 02:30:21, dcheng wrote: > > On 2016/09/30 02:24:52, Nico (mostly away until Oct 3) wrote: > > > > > > https://codereview.chromium.org/2380003003/diff/1/third_party/WebKit/Source/c... > > > File third_party/WebKit/Source/core/frame/PRESUBMIT.py (right): > > > > > > > > > https://codereview.chromium.org/2380003003/diff/1/third_party/WebKit/Source/c... > > > third_party/WebKit/Source/core/frame/PRESUBMIT.py:25: r'case > > > CSSProperty\w*?\:\s*?\n?\s+?return (\d+);', > > > On 2016/09/30 01:56:38, dcheng wrote: > > > > On 2016/09/30 00:41:18, Nico (mostly away until Oct 3) wrote: > > > > > why \s+ instead of \s* ? > > > > > > > > > > So `return\n43;` doesn't happen in practice? > > > > > > > > I think \s will match \n, since we're matching in multiline mode. I > > simplified > > > > it a bit overall by replacing \s*?\n?\s+ with just \s+? > > > > > > I mean isn't `\s+?` the same as `\s*` ? > > > > + is 1 or more, * is 0 or more. I don't think it'll happen, but in case > someone > > somehow uses return123124 in an identifier, we don't want to match that. > > Well yeah, but if you put ? after \s+ it means "one or more, or not" which seems > like the same as "zero or more" Ah. So ? combined with +, *, or ? turns it into a non greedy version. By default, +, *, and ? match as much as they can. In contrast, +? match as little as possible. That being said, using the non greedy version here doesn't really matter, and I probably should have just used the greedy one. |
