Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1407093002:
media/PRESUBMIT.py: adding CheckPassByValue for base::Time and derived classes (Closed)
|
Created:
5 years, 2 months ago by mcasas Modified:
4 years, 8 months ago Reviewers:
sandersd (OOO until July 31) CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmedia/PRESUBMIT.py: adding CheckPassByValue for base::Time and derived classes
This bites me every time, that base::Time, TimeTicks,
TimeDelta should be passed by value, not by const&.
The original function is from cc/PRESUBMIT.py and
copied nearly-verbatim here (removed the white_list
and black_list parameters and checking all CL code
files instead).
TEST= Only way I could was to add a method somewhere
with a const base::TimeTicks& as argument and running
a git cl presubmit, which found this and complained.
Committed: https://crrev.com/e60e4f755c905b7a86dd967767ffc550714f60b6
Cr-Commit-Position: refs/heads/master@{#384769}
Patch Set 1 : tested #
Total comments: 12
Patch Set 2 : sandersd@ comments #Patch Set 3 : second round of comments #
Total comments: 8
Patch Set 4 : third round of comments!! #
Total comments: 4
Patch Set 5 : #
Total comments: 2
Patch Set 6 : PASS_BY_VALUE_RE--> BASE_TIME_TYPES_RE #Messages
Total messages: 27 (7 generated)
Patchset #1 (id:1) has been deleted
mcasas@chromium.org changed reviewers: + sandersd@chromium.org
sandersd@ PTAL
The CQ bit was checked by mcasas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1407093002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1407093002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1407093002/diff/20001/media/PRESUBMIT.py File media/PRESUBMIT.py (right): https://codereview.chromium.org/1407093002/diff/20001/media/PRESUBMIT.py#newc... media/PRESUBMIT.py:164: def _CheckPassByValue(input_api, No need to wrap arguments. https://codereview.chromium.org/1407093002/diff/20001/media/PRESUBMIT.py#newc... media/PRESUBMIT.py:176: r'\bconst +' + '(?P<type>(%s))&' % This regex should be precompiled, eg: PASS_BY_VALUE_RE = re.compile(r"...") def _CheckPassByValue(...): match = PASS_BY_VALUE_RE.search(contents) https://codereview.chromium.org/1407093002/diff/20001/media/PRESUBMIT.py#newc... media/PRESUBMIT.py:177: string.join(pass_by_value_types, '|'), Instead of building 'const foo&|cost bar&', it would make more sense to build 'cost (foo|bar)&'. This would remove the need for a group name and make it much easier to read. https://codereview.chromium.org/1407093002/diff/20001/media/PRESUBMIT.py#newc... media/PRESUBMIT.py:179: if match: Note that there is also .findall(), which iterates over all non-overlapping matches. That may be better if the goal is to list all instances to be fixed.
PTAL https://chromiumcodereview.appspot.com/1407093002/diff/20001/media/PRESUBMIT.py File media/PRESUBMIT.py (right): https://chromiumcodereview.appspot.com/1407093002/diff/20001/media/PRESUBMIT.... media/PRESUBMIT.py:164: def _CheckPassByValue(input_api, On 2015/10/16 00:54:31, sandersd wrote: > No need to wrap arguments. Done. https://chromiumcodereview.appspot.com/1407093002/diff/20001/media/PRESUBMIT.... media/PRESUBMIT.py:176: r'\bconst +' + '(?P<type>(%s))&' % On 2015/10/16 00:54:31, sandersd wrote: > This regex should be precompiled, eg: > > PASS_BY_VALUE_RE = re.compile(r"...") > > > def _CheckPassByValue(...): > match = PASS_BY_VALUE_RE.search(contents) Done. https://chromiumcodereview.appspot.com/1407093002/diff/20001/media/PRESUBMIT.... media/PRESUBMIT.py:177: string.join(pass_by_value_types, '|'), On 2015/10/16 00:54:31, sandersd wrote: > Instead of building 'const foo&|cost bar&', it would make more sense to build > 'cost (foo|bar)&'. This would remove the need for a group name and make it much > easier to read. Isn't it what's done? Regexp is r'... <type>(%s))&' % string.join(pass_by_value_types, '|') expands to: r'... <type>(%s))&' % 'base::Time|base::TimeTicks' which expands to: r'... <type>(base::Time|base::TimeTicks))&' https://chromiumcodereview.appspot.com/1407093002/diff/20001/media/PRESUBMIT.... media/PRESUBMIT.py:179: if match: On 2015/10/16 00:54:31, sandersd wrote: > Note that there is also .findall(), which iterates over all non-overlapping > matches. That may be better if the goal is to list all instances to be fixed. My first goal was to stick as much as possible to the cc/PRESUBMIT.py version. It's true that this code would only list the file while not indicating the line for the syntax bug and/or if there's one or multiple hits. WDYT? Should I make this code more sophisticated? Or does it help enough 'as is'?
https://chromiumcodereview.appspot.com/1407093002/diff/20001/media/PRESUBMIT.py File media/PRESUBMIT.py (right): https://chromiumcodereview.appspot.com/1407093002/diff/20001/media/PRESUBMIT.... media/PRESUBMIT.py:177: string.join(pass_by_value_types, '|'), On 2015/10/16 18:00:11, mcasas wrote: > On 2015/10/16 00:54:31, sandersd wrote: > > Instead of building 'const foo&|cost bar&', it would make more sense to build > > 'cost (foo|bar)&'. This would remove the need for a group name and make it > much > > easier to read. > > Isn't it what's done? Regexp is > > r'... <type>(%s))&' % string.join(pass_by_value_types, '|') > > expands to: > > r'... <type>(%s))&' % 'base::Time|base::TimeTicks' > > which expands to: > > r'... <type>(base::Time|base::TimeTicks))&' > > > Hmm, you're correct, but that means there is no need for naming the capture group in the current form. It's syntactic noise I'd rather was removed. https://chromiumcodereview.appspot.com/1407093002/diff/20001/media/PRESUBMIT.... media/PRESUBMIT.py:179: if match: On 2015/10/16 18:00:11, mcasas wrote: > On 2015/10/16 00:54:31, sandersd wrote: > > Note that there is also .findall(), which iterates over all non-overlapping > > matches. That may be better if the goal is to list all instances to be fixed. > > My first goal was to stick as much as possible to the > cc/PRESUBMIT.py version. It's true that this code > would only list the file while not indicating the line > for the syntax bug and/or if there's one or multiple > hits. WDYT? Should I make this code more > sophisticated? Or does it help enough 'as is'? Probably best to list them all, it's a trivial change.
PTAL https://chromiumcodereview.appspot.com/1407093002/diff/20001/media/PRESUBMIT.py File media/PRESUBMIT.py (right): https://chromiumcodereview.appspot.com/1407093002/diff/20001/media/PRESUBMIT.... media/PRESUBMIT.py:177: string.join(pass_by_value_types, '|'), On 2015/10/16 18:27:12, sandersd wrote: > On 2015/10/16 18:00:11, mcasas wrote: > > On 2015/10/16 00:54:31, sandersd wrote: > > > Instead of building 'const foo&|cost bar&', it would make more sense to > build > > > 'cost (foo|bar)&'. This would remove the need for a group name and make it > > much > > > easier to read. > > > > Isn't it what's done? Regexp is > > > > r'... <type>(%s))&' % string.join(pass_by_value_types, '|') > > > > expands to: > > > > r'... <type>(%s))&' % 'base::Time|base::TimeTicks' > > > > which expands to: > > > > r'... <type>(base::Time|base::TimeTicks))&' > > > > > > > > Hmm, you're correct, but that means there is no need for naming the capture > group in the current form. It's syntactic noise I'd rather was removed. Done. https://chromiumcodereview.appspot.com/1407093002/diff/20001/media/PRESUBMIT.... media/PRESUBMIT.py:179: if match: On 2015/10/16 18:27:12, sandersd wrote: > On 2015/10/16 18:00:11, mcasas wrote: > > On 2015/10/16 00:54:31, sandersd wrote: > > > Note that there is also .findall(), which iterates over all non-overlapping > > > matches. That may be better if the goal is to list all instances to be > fixed. > > > > My first goal was to stick as much as possible to the > > cc/PRESUBMIT.py version. It's true that this code > > would only list the file while not indicating the line > > for the syntax bug and/or if there's one or multiple > > hits. WDYT? Should I make this code more > > sophisticated? Or does it help enough 'as is'? > > Probably best to list them all, it's a trivial change. Not that trivial since find_all() would not give the line number, and we would like to give that information, correct? I rewrote the whole routine to look more like the neighbours.
https://chromiumcodereview.appspot.com/1407093002/diff/60001/media/PRESUBMIT.py File media/PRESUBMIT.py (right): https://chromiumcodereview.appspot.com/1407093002/diff/60001/media/PRESUBMIT.... media/PRESUBMIT.py:18: ] Can you check that pyformat agrees with this? If not, I prefer: PASS_BY_VALUE_TYPES = [ 'base::Time', 'base::TimeDelta', 'base::TimeTicks', ] https://chromiumcodereview.appspot.com/1407093002/diff/60001/media/PRESUBMIT.... media/PRESUBMIT.py:20: PASS_BY_VALUE_RE = re.compile(r'\bconst +' + '%s&' % This requires parentheses around %s. https://chromiumcodereview.appspot.com/1407093002/diff/60001/media/PRESUBMIT.... media/PRESUBMIT.py:179: if PASS_BY_VALUE_RE.search(line.strip()): No need to strip the line here. https://chromiumcodereview.appspot.com/1407093002/diff/60001/media/PRESUBMIT.... media/PRESUBMIT.py:181: ' %s:%d passes argument by const ref instead of by value:\n %s' % I'm not too happy with this message because it doesn't actually tell you precisely what to fix to make it happy. I'd rather include the name of the class and rephrase as an instruction to fix it.
PTAL https://chromiumcodereview.appspot.com/1407093002/diff/60001/media/PRESUBMIT.py File media/PRESUBMIT.py (right): https://chromiumcodereview.appspot.com/1407093002/diff/60001/media/PRESUBMIT.... media/PRESUBMIT.py:18: ] On 2015/10/16 21:52:27, sandersd wrote: > Can you check that pyformat agrees with this? If not, I prefer: > > PASS_BY_VALUE_TYPES = [ > 'base::Time', > 'base::TimeDelta', > 'base::TimeTicks', > ] If you're happy like that, I'm happy too. Done. https://chromiumcodereview.appspot.com/1407093002/diff/60001/media/PRESUBMIT.... media/PRESUBMIT.py:20: PASS_BY_VALUE_RE = re.compile(r'\bconst +' + '%s&' % On 2015/10/16 21:52:27, sandersd wrote: > This requires parentheses around %s. Done. https://chromiumcodereview.appspot.com/1407093002/diff/60001/media/PRESUBMIT.... media/PRESUBMIT.py:179: if PASS_BY_VALUE_RE.search(line.strip()): On 2015/10/16 21:52:27, sandersd wrote: > No need to strip the line here. Done. https://chromiumcodereview.appspot.com/1407093002/diff/60001/media/PRESUBMIT.... media/PRESUBMIT.py:181: ' %s:%d passes argument by const ref instead of by value:\n %s' % On 2015/10/16 21:52:27, sandersd wrote: > I'm not too happy with this message because it doesn't actually tell you > precisely what to fix to make it happy. I'd rather include the name of the class > and rephrase as an instruction to fix it. Rephrase to make it look more like the neighbours. Please suggest alternative otherwise...
https://chromiumcodereview.appspot.com/1407093002/diff/80001/media/PRESUBMIT.py File media/PRESUBMIT.py (right): https://chromiumcodereview.appspot.com/1407093002/diff/80001/media/PRESUBMIT.... media/PRESUBMIT.py:15: PASS_BY_VALUE_TYPES = [ Based on the message I'd rename this as BASE_TIME_TYPES. https://chromiumcodereview.appspot.com/1407093002/diff/80001/media/PRESUBMIT.... media/PRESUBMIT.py:21: PASS_BY_VALUE_RE = re.compile(r'\bconst +' + '(%s)&' % Something's gone wrong here, should be: re.compile(r'\bconst (%s)&' %
https://chromiumcodereview.appspot.com/1407093002/diff/80001/media/PRESUBMIT.py File media/PRESUBMIT.py (right): https://chromiumcodereview.appspot.com/1407093002/diff/80001/media/PRESUBMIT.... media/PRESUBMIT.py:15: PASS_BY_VALUE_TYPES = [ On 2015/10/20 00:29:08, sandersd wrote: > Based on the message I'd rename this as BASE_TIME_TYPES. Done. https://chromiumcodereview.appspot.com/1407093002/diff/80001/media/PRESUBMIT.... media/PRESUBMIT.py:21: PASS_BY_VALUE_RE = re.compile(r'\bconst +' + '(%s)&' % On 2015/10/20 00:29:08, sandersd wrote: > Something's gone wrong here, should be: > > re.compile(r'\bconst (%s)&' % Done.
https://chromiumcodereview.appspot.com/1407093002/diff/100001/media/PRESUBMIT.py File media/PRESUBMIT.py (right): https://chromiumcodereview.appspot.com/1407093002/diff/100001/media/PRESUBMIT... media/PRESUBMIT.py:21: PASS_BY_VALUE_RE = re.compile(r'\bconst (%s)&' % This should be BASE_TIME_TYPES_RE to match.
On 2015/10/20 21:01:03, sandersd wrote: > https://chromiumcodereview.appspot.com/1407093002/diff/100001/media/PRESUBMIT.py > File media/PRESUBMIT.py (right): > > https://chromiumcodereview.appspot.com/1407093002/diff/100001/media/PRESUBMIT... > media/PRESUBMIT.py:21: PASS_BY_VALUE_RE = re.compile(r'\bconst (%s)&' % > This should be BASE_TIME_TYPES_RE to match. sandersd@: it's pretty frustrating that you keep giving me little review bits, e.g. if this is your only remark, why not LGTM % comment? Furthermore, precompiling the re. expression is not commonplace of this file, so I'm fearing e.g. your next review would ask me to add a crbug.com for it etc. Finally, I'm also starting to doubt that you're convinced about the utility and usefulness of this patch.
On 2015/10/20 21:33:20, mcasas wrote: > On 2015/10/20 21:01:03, sandersd wrote: > > > https://chromiumcodereview.appspot.com/1407093002/diff/100001/media/PRESUBMIT.py > > File media/PRESUBMIT.py (right): > > > > > https://chromiumcodereview.appspot.com/1407093002/diff/100001/media/PRESUBMIT... > > media/PRESUBMIT.py:21: PASS_BY_VALUE_RE = re.compile(r'\bconst (%s)&' % > > This should be BASE_TIME_TYPES_RE to match. > > sandersd@: it's pretty frustrating that you keep giving > me little review bits, e.g. if this is your only remark, why > not LGTM % comment? Furthermore, precompiling > the re. expression is not commonplace of this file, so > I'm fearing e.g. your next review would ask me to add > a http://crbug.com for it etc. Finally, I'm also starting to > doubt that you're convinced about the utility and > usefulness of this patch. The truth is that I don't know why you didn't change both names at the same time. I don't have other requests. I didn't % remark because previously in this review you have made larger changes than requested, and so I want the option to see what changes are actually made. I agree that this process has had a long sequence of small changes, and I realize that that is frustrating. I hope you agree that the design changed (vs PS 1) and that my exact list of original nits isn't applicable to the final result.
sanders@ I resuscitated this CL that was somewhere zombiefid, could you PTAL? thanks! https://codereview.chromium.org/1407093002/diff/100001/media/PRESUBMIT.py File media/PRESUBMIT.py (right): https://codereview.chromium.org/1407093002/diff/100001/media/PRESUBMIT.py#new... media/PRESUBMIT.py:21: PASS_BY_VALUE_RE = re.compile(r'\bconst (%s)&' % On 2015/10/20 21:01:03, sandersd wrote: > This should be BASE_TIME_TYPES_RE to match. Done.
lgtm
On 2016/04/01 21:36:23, sandersd wrote: > lgtm \o/
The CQ bit was checked by mcasas@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1407093002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1407093002/120001
Message was sent while issue was closed.
Description was changed from ========== media/PRESUBMIT.py: adding CheckPassByValue for base::Time and derived classes This bites me every time, that base::Time, TimeTicks, TimeDelta should be passed by value, not by const&. The original function is from cc/PRESUBMIT.py and copied nearly-verbatim here (removed the white_list and black_list parameters and checking all CL code files instead). TEST= Only way I could was to add a method somewhere with a const base::TimeTicks& as argument and running a git cl presubmit, which found this and complained. ========== to ========== media/PRESUBMIT.py: adding CheckPassByValue for base::Time and derived classes This bites me every time, that base::Time, TimeTicks, TimeDelta should be passed by value, not by const&. The original function is from cc/PRESUBMIT.py and copied nearly-verbatim here (removed the white_list and black_list parameters and checking all CL code files instead). TEST= Only way I could was to add a method somewhere with a const base::TimeTicks& as argument and running a git cl presubmit, which found this and complained. ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== media/PRESUBMIT.py: adding CheckPassByValue for base::Time and derived classes This bites me every time, that base::Time, TimeTicks, TimeDelta should be passed by value, not by const&. The original function is from cc/PRESUBMIT.py and copied nearly-verbatim here (removed the white_list and black_list parameters and checking all CL code files instead). TEST= Only way I could was to add a method somewhere with a const base::TimeTicks& as argument and running a git cl presubmit, which found this and complained. ========== to ========== media/PRESUBMIT.py: adding CheckPassByValue for base::Time and derived classes This bites me every time, that base::Time, TimeTicks, TimeDelta should be passed by value, not by const&. The original function is from cc/PRESUBMIT.py and copied nearly-verbatim here (removed the white_list and black_list parameters and checking all CL code files instead). TEST= Only way I could was to add a method somewhere with a const base::TimeTicks& as argument and running a git cl presubmit, which found this and complained. Committed: https://crrev.com/e60e4f755c905b7a86dd967767ffc550714f60b6 Cr-Commit-Position: refs/heads/master@{#384769} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/e60e4f755c905b7a86dd967767ffc550714f60b6 Cr-Commit-Position: refs/heads/master@{#384769} |
