Presubmit enforce that DCHECK_IS_ON() does not forget the braces.

Presubmit enforce that DCHECK_IS_ON() does not forget the braces.

If you forget the braces, then it will just ignore what was inside
the #if, which is not what you wanted.

Long awaited followup to https://codereview.chromium.org/842523002/

R=phajdan.jr@chromium.org

Committed: https://crrev.com/61c1aa285420ab0de1e1b0777ae43f211766a6c5
Cr-Commit-Position: refs/heads/master@{#356099}

[ncarter (slow), 2015-10-23 20:39:02]

nick@chromium.org changed reviewers:
+ nick@chromium.org

[ncarter (slow), 2015-10-23 20:39:02]

https://codereview.chromium.org/1407033004/diff/1/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/1407033004/diff/1/PRESUBMIT.py#newcode378
PRESUBMIT.py:378: """Checks to make sure DCHECK_IS_ON() does not skip the
braces"""
Nit: period at end of sentence per
https://www.python.org/dev/peps/pep-0257/#id16 (via the python style guide).

https://codereview.chromium.org/1407033004/diff/1/PRESUBMIT.py#newcode385
PRESUBMIT.py:385: contents = input_api.ReadFile(f)
Should this iterate over ChangedContents (the diff lines) instead of the whole
file?

[danakj, 2015-10-23 21:53:16]

PTAL

https://codereview.chromium.org/1407033004/diff/1/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/1407033004/diff/1/PRESUBMIT.py#newcode378
PRESUBMIT.py:378: """Checks to make sure DCHECK_IS_ON() does not skip the
braces"""
On 2015/10/23 20:39:02, ncarter wrote:
> Nit: period at end of sentence per
> https://www.python.org/dev/peps/pep-0257/#id16 (via the python style guide).

Done and fixed what I copied from.

https://codereview.chromium.org/1407033004/diff/1/PRESUBMIT.py#newcode385
PRESUBMIT.py:385: contents = input_api.ReadFile(f)
On 2015/10/23 20:39:02, ncarter wrote:
> Should this iterate over ChangedContents (the diff lines) instead of the whole
> file?

Ya good point, copied from the UNIT_TEST one. Fixed.

[danakj, 2015-10-23 21:53:42]

https://codereview.chromium.org/1407033004/diff/20001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/1407033004/diff/20001/PRESUBMIT.py#newcode388
PRESUBMIT.py:388: ('%s:%d: Use of DCHECK_IS_ON() must be written as "#if ' +
The error is nicer now too :)

[ncarter (slow), 2015-10-23 22:01:52]

lgtm

[ncarter (slow), 2015-10-23 22:04:19]

On 2015/10/23 22:01:52, ncarter wrote:
> lgtm

I should add: while this is not a super commonly used macro, we've seen bugs
with it occur more than once in practice, so I think there's a reasonable chance
that this PRESUBMIT rule will prevent future instances, and is worth its cost.

[Nico, 2015-10-23 23:06:53]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-10-23 23:06:53]

can you check how much this slows down the presubmit step? (it's already way to
slow, because we already have too many presubmits :-/)

[danakj, 2015-10-24 00:14:05]

On 2015/10/23 23:06:53, Nico (vacation Fri Oct 23) wrote:
> can you check how much this slows down the presubmit step? (it's already way
to
> slow, because we already have too many presubmits :-/)

PRESUBMIT is kinda broken, it runs over the N lines of code M times for M
checks. We should write checks as hooks and then walk over the code once and run
all the checks as it goes. But no matter what a few seconds on upload is worth
it for saving reviews time and preventing bugs in basically every case IMO.
</rant>

The change is within noise for a single change. It's running a regex over the
diff.
With: real	0m6.783s
Without: real	0m6.850s
Without: real	0m6.725s

I tried a pretty big CL I've been working on..
Without: real	0m15.405s
Presubmit checks took 12.4s to calculate.
With: real	0m15.016s
Presubmit checks took 12.1s to calculate.

I can't see a diff.

[ncarter (slow), 2015-10-24 01:49:40]

On 2015/10/24 00:14:05, danakj wrote:
> On 2015/10/23 23:06:53, Nico (vacation Fri Oct 23) wrote:
> > can you check how much this slows down the presubmit step? (it's already way
> to
> > slow, because we already have too many presubmits :-/)
> 
> PRESUBMIT is kinda broken, it runs over the N lines of code M times for M
> checks. We should write checks as hooks and then walk over the code once and
run
> all the checks as it goes. But no matter what a few seconds on upload is worth
> it for saving reviews time and preventing bugs in basically every case IMO.
> </rant>
> 
> The change is within noise for a single change. It's running a regex over the
> diff.
> With: real	0m6.783s
> Without: real	0m6.850s
> Without: real	0m6.725s
> 
> I tried a pretty big CL I've been working on..
> Without: real	0m15.405s
> Presubmit checks took 12.4s to calculate.
> With: real	0m15.016s
> Presubmit checks took 12.1s to calculate.
> 
> I can't see a diff.

I also ran some numbers here (since I've also been bothered by slow presubmits);
I was testing with a a 52-file, +1400, -1700 line change.

* "git cl presubmit" currently takes 4.7 seconds on my machine. Adding this new
check costs an additional 5.83 ms (a 0.12% slowdown). Of that 5.83ms, approx 40%
is inside the input_api.re.search() call. Surprisingly (to me), re.compile seems
to be cheap enough as to be negligible

* Iterating over NewContents() would have been about 9x slower on this new
check. both NewContents() and ChangedContents() are cached under the hood, and
the caches are shared between presubmit functions)

* _CheckNoPragmaOnce uses an interesting pattern where it does ReadFile() (which
isn't cached) and then does a regex on the whole file. This pattern seems to be
significantly (2x) faster than iterating over NewContents(), because there's
only one call to re.search per file, but still 5x slower than just looking at
the diffs.

* For comparison, an expensive existing regex style check,
_CheckNoBannedFunctions, seems to cost 110ms.

[ Methodology: I wrapped the innards of select presubmit checks in a "for x in
xrange(10000):", and observed the effects of (a) having a different regex each
loop iteration, or hoisting it out and (b) using two precompiled regexes for
each line in the file. ]

With some effort I think we could make these regex cases screamingly fast. But
new simple ones are pretty cheap incrementally, if they're written properly.

_CheckNoBannedFunctions could be made faster by |-ing together the pattern list
(and using named capture groups to figure out which subpatterns match). You
could then definitely generalize that and zipper together all simple regex
checks. It would also be really efficient to just to run the regex on the
concatenation on the raw SCM diff (and then filter out matches that don't start
with '+'), but to get line numbers, you need to be matching whole diff hunks,
including the header. Not having to worry about svn support probably helps.

[ncarter (slow), 2015-10-24 06:51:41]

On 2015/10/24 01:49:40, ncarter wrote:
> On 2015/10/24 00:14:05, danakj wrote:
> > On 2015/10/23 23:06:53, Nico (vacation Fri Oct 23) wrote:
> > > can you check how much this slows down the presubmit step? (it's already
way
> > to
> > > slow, because we already have too many presubmits :-/)
> > 
> > PRESUBMIT is kinda broken, it runs over the N lines of code M times for M
> > checks. We should write checks as hooks and then walk over the code once and
> run
> > all the checks as it goes. But no matter what a few seconds on upload is
worth
> > it for saving reviews time and preventing bugs in basically every case IMO.
> > </rant>
> > 
> > The change is within noise for a single change. It's running a regex over
the
> > diff.
> > With: real	0m6.783s
> > Without: real	0m6.850s
> > Without: real	0m6.725s
> > 
> > I tried a pretty big CL I've been working on..
> > Without: real	0m15.405s
> > Presubmit checks took 12.4s to calculate.
> > With: real	0m15.016s
> > Presubmit checks took 12.1s to calculate.
> > 
> > I can't see a diff.
> 
> I also ran some numbers here (since I've also been bothered by slow
presubmits);
> I was testing with a a 52-file, +1400, -1700 line change.
> 
> * "git cl presubmit" currently takes 4.7 seconds on my machine. Adding this
new
> check costs an additional 5.83 ms (a 0.12% slowdown). Of that 5.83ms, approx
40%
> is inside the input_api.re.search() call. Surprisingly (to me), re.compile
seems
> to be cheap enough as to be negligible
> 
> * Iterating over NewContents() would have been about 9x slower on this new
> check. both NewContents() and ChangedContents() are cached under the hood, and
> the caches are shared between presubmit functions)
> 
> * _CheckNoPragmaOnce uses an interesting pattern where it does ReadFile()
(which
> isn't cached) and then does a regex on the whole file. This pattern seems to
be
> significantly (2x) faster than iterating over NewContents(), because there's
> only one call to re.search per file, but still 5x slower than just looking at
> the diffs.
> 
> * For comparison, an expensive existing regex style check,
> _CheckNoBannedFunctions, seems to cost 110ms.
> 
> [ Methodology: I wrapped the innards of select presubmit checks in a "for x in
> xrange(10000):", and observed the effects of (a) having a different regex each
> loop iteration, or hoisting it out and (b) using two precompiled regexes for
> each line in the file. ]
> 
> With some effort I think we could make these regex cases screamingly fast. But
> new simple ones are pretty cheap incrementally, if they're written properly.
> 
> _CheckNoBannedFunctions could be made faster by |-ing together the pattern
list
> (and using named capture groups to figure out which subpatterns match). You
> could then definitely generalize that and zipper together all simple regex
> checks. It would also be really efficient to just to run the regex on the
> concatenation on the raw SCM diff (and then filter out matches that don't
start
> with '+'), but to get line numbers, you need to be matching whole diff hunks,
> including the header. Not having to worry about svn support probably helps.

FYI: further digging led to
https://code.google.com/p/chromium/issues/detail?id=547321 . Dunno if it's as
big a problem on other platforms, but I am guessing it's been exacerbated by the
blink merge.

[Paweł Hajdan Jr., 2015-10-26 12:53:36]

LGTM

[danakj, 2015-10-26 18:14:15]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2015-10-26 18:15:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1407033004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1407033004/20001

[commit-bot: I haz the power, 2015-10-26 18:19:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-26 18:19:46]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[danakj, 2015-10-26 19:51:16]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2015-10-26 19:51:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1407033004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1407033004/20001

[commit-bot: I haz the power, 2015-10-26 19:55:59]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-10-26 19:56:48]

Patchset 2 (id:??) landed as
https://crrev.com/61c1aa285420ab0de1e1b0777ae43f211766a6c5
Cr-Commit-Position: refs/heads/master@{#356099}

[kjellander_chromium, 2017-02-20 07:09:47]

kjellander@chromium.org changed reviewers:
+ kjellander@chromium.org

[kjellander_chromium, 2017-02-20 07:09:49]

AFAIK braces look like this: {} (a.k.a. curly brackets) and () is always called
parentheses.

https://codereview.chromium.org/1407033004/diff/20001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/1407033004/diff/20001/PRESUBMIT.py#newcode389
PRESUBMIT.py:389: 'DCHECK_IS_ON()", not forgetting the braces.')
Shouldn't 'braces' be replaced by 'parentheses' or am I missing something?
See http://practicaltypography.com/parentheses-brackets-and-braces.html

[danakj, 2017-02-22 00:19:54]

https://codereview.chromium.org/1407033004/diff/20001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/1407033004/diff/20001/PRESUBMIT.py#newcode389
PRESUBMIT.py:389: 'DCHECK_IS_ON()", not forgetting the braces.')
On 2017/02/20 07:09:48, kjellander_chromium wrote:
> Shouldn't 'braces' be replaced by 'parentheses' or am I missing something?
> See http://practicaltypography.com/parentheses-brackets-and-braces.html

You can submit a CL to fix it if you want :)

[kjellander_chromium, 2017-02-22 11:08:18]

On 2017/02/22 00:19:54, danakj wrote:
> https://codereview.chromium.org/1407033004/diff/20001/PRESUBMIT.py
> File PRESUBMIT.py (right):
> 
> https://codereview.chromium.org/1407033004/diff/20001/PRESUBMIT.py#newcode389
> PRESUBMIT.py:389: 'DCHECK_IS_ON()", not forgetting the braces.')
> On 2017/02/20 07:09:48, kjellander_chromium wrote:
> > Shouldn't 'braces' be replaced by 'parentheses' or am I missing something?
> > See http://practicaltypography.com/parentheses-brackets-and-braces.html
> 
> You can submit a CL to fix it if you want :)

Sure: https://codereview.chromium.org/2705303004/