Add PRESUBMIT.py warning for contradictory NOTREACHED() use.

Add PRESUBMIT.py warning for contradictory NOTREACHED() use.

As recently discussed on chromium-dev [1] and subsequently incorporated into the
Chromium Coding Style [2], NOTREACHED() followed by code is an anti-pattern.
This CL lets PRESUBMIT.py detect most of these cases and print a warning to the
developer.
[1] https://groups.google.com/a/chromium.org/d/msg/chromium-dev/c2X0D1Z6X2o/ddkFLqQP0P0J
[2] http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-

BUG=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=279681

[Thiemo Nagel, 2014-06-18 17:16:41]

Hi Peter,

after our discussion in MTV last week, would you mind to take a look?

Thank you!
Thiemo

[Peter Kasting, 2014-06-18 18:06:14]

Unfortunately I'm not capable of reviewing Python changes; please send to
someone who has done nontrivial work on this file before.

[Thiemo Nagel, 2014-06-18 19:23:36]

Hi Marc-Antoine,

could you please be so kind to take a look at this CL which is intended to nag
developers when they're using NOTREACHED() improperly?

Thank you!
Thiemo

[M-A Ruel, 2014-06-18 19:35:11]

It's a bit on the heavy side for processing but I don't mind that much.

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

https://codereview.chromium.org/344563003/diff/1/PRESUBMIT.py#newcode1106
PRESUBMIT.py:1106: 
2 lines between file level symbols

https://codereview.chromium.org/344563003/diff/1/PRESUBMIT.py#newcode1109
PRESUBMIT.py:1109: r".+\.c$", r".+\.cc$", r".+\.cpp$", r".+\.h$", r".+\.hpp$" )
.m and .mm and .inl

https://codereview.chromium.org/344563003/diff/1/PRESUBMIT.py#newcode1115
PRESUBMIT.py:1115: results += _CheckContradictoryNotreachedUseInFile(input_api,
fpath)
use extend()

https://codereview.chromium.org/344563003/diff/1/PRESUBMIT.py#newcode1116
PRESUBMIT.py:1116: return [ output_api.PresubmitPromptWarning(r) for r in
results ]
no space around []
or if you want:
return map(output_api.PresubmitPromptWarning, results)
but that's borderline cheezy.

https://codereview.chromium.org/344563003/diff/1/PRESUBMIT.py#newcode1124
PRESUBMIT.py:1124: for line in contents:
You don't use contents after and you just join so:

text = ''.join(line + '\n' for line in f.NewContents())

https://codereview.chromium.org/344563003/diff/1/PRESUBMIT.py#newcode1131
PRESUBMIT.py:1131: r'[
\t]*NOTREACHED\(\s*\).*?;(?P<between>.*?)((\bbreak\b)|})',
align arguments at +4

[Thiemo Nagel, 2014-06-24 12:07:38]

Many thanks for your review!  I've addressed your comments, could you please
take another look?

Concerning execution speed, I've removed one of the passes over the data (for
line ends).  I'm new to Python but I guess that further optimizations can be
made if speed turns out to be a problem.

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

https://codereview.chromium.org/344563003/diff/1/PRESUBMIT.py#newcode1106
PRESUBMIT.py:1106: 
On 2014/06/18 19:35:11, M-A Ruel wrote:
> 2 lines between file level symbols

Done.

https://codereview.chromium.org/344563003/diff/1/PRESUBMIT.py#newcode1109
PRESUBMIT.py:1109: r".+\.c$", r".+\.cc$", r".+\.cpp$", r".+\.h$", r".+\.hpp$" )
On 2014/06/18 19:35:11, M-A Ruel wrote:
> .m and .mm and .inl

Done.

https://codereview.chromium.org/344563003/diff/1/PRESUBMIT.py#newcode1115
PRESUBMIT.py:1115: results += _CheckContradictoryNotreachedUseInFile(input_api,
fpath)
On 2014/06/18 19:35:11, M-A Ruel wrote:
> use extend()

Done.

https://codereview.chromium.org/344563003/diff/1/PRESUBMIT.py#newcode1116
PRESUBMIT.py:1116: return [ output_api.PresubmitPromptWarning(r) for r in
results ]
On 2014/06/18 19:35:11, M-A Ruel wrote:
> no space around []
> or if you want:
> return map(output_api.PresubmitPromptWarning, results)
> but that's borderline cheezy.

Done.

https://codereview.chromium.org/344563003/diff/1/PRESUBMIT.py#newcode1124
PRESUBMIT.py:1124: for line in contents:
On 2014/06/18 19:35:11, M-A Ruel wrote:
> You don't use contents after and you just join so:
> 
> text = ''.join(line + '\n' for line in f.NewContents())

Done.

https://codereview.chromium.org/344563003/diff/1/PRESUBMIT.py#newcode1131
PRESUBMIT.py:1131: r'[
\t]*NOTREACHED\(\s*\).*?;(?P<between>.*?)((\bbreak\b)|})',
On 2014/06/18 19:35:11, M-A Ruel wrote:
> align arguments at +4

Done.

[M-A Ruel, 2014-06-24 17:45:22]

lgtm

https://codereview.chromium.org/344563003/diff/40001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/344563003/diff/40001/PRESUBMIT.py#newcode1067
PRESUBMIT.py:1067: input data is formatted with unix-style line ends."""
The CRLF -> LF reduction is unnecessary as per 
https://chromium.googlesource.com/chromium/tools/depot_tools.git/+/master/pre...
'rU' means python does the conversion for you.

https://codereview.chromium.org/344563003/diff/40001/PRESUBMIT.py#newcode1123
PRESUBMIT.py:1123: 'http://www.chromium.org/developers/coding-style'
optional: you can skip 'www.' to save a few chars if you want.

https://codereview.chromium.org/344563003/diff/40001/PRESUBMIT.py#newcode1143
PRESUBMIT.py:1143: excerpt = excerpt[:100] + ' ...'
nit: Replace ' ...' with '\u2026'. It should work fine even on Windows.

[Thiemo Nagel, 2014-06-25 06:45:54]

Thank you!

https://codereview.chromium.org/344563003/diff/40001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/344563003/diff/40001/PRESUBMIT.py#newcode1067
PRESUBMIT.py:1067: input data is formatted with unix-style line ends."""
On 2014/06/24 17:45:22, M-A Ruel wrote:
> The CRLF -> LF reduction is unnecessary as per 
>
https://chromium.googlesource.com/chromium/tools/depot_tools.git/+/master/pre...
> 'rU' means python does the conversion for you.

Thanks.  I was foiled into believing \r wasn't stripped because MockFile doesn't
do it.

https://codereview.chromium.org/344563003/diff/40001/PRESUBMIT.py#newcode1123
PRESUBMIT.py:1123: 'http://www.chromium.org/developers/coding-style'
On 2014/06/24 17:45:22, M-A Ruel wrote:
> optional: you can skip 'www.' to save a few chars if you want.

Done.

https://codereview.chromium.org/344563003/diff/40001/PRESUBMIT.py#newcode1143
PRESUBMIT.py:1143: excerpt = excerpt[:100] + ' ...'
On 2014/06/24 17:45:22, M-A Ruel wrote:
> nit: Replace ' ...' with '\u2026'. It should work fine even on Windows.

Done.

[Thiemo Nagel, 2014-06-25 06:48:52]

The CQ bit was checked by tnagel@chromium.org

[commit-bot: I haz the power, 2014-06-25 06:51:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tnagel@chromium.org/344563003/80001

[commit-bot: I haz the power, 2014-06-25 11:55:25]

Change committed as 279681

[M-A Ruel, 2014-06-25 13:13:36]

On 2014/06/25 11:55:25, I haz the power (commit-bot) wrote:
> Change committed as 279681

FTR, PRESUBMIT only CLs should use NOTRY=true, as described at
https://sites.google.com/a/chromium.org/dev/developers/testing/commit-queue?p....
There's no point in running tests on this change, so you can use this next time
to have it be committed faster.

[Miguel Garcia, 2014-06-25 14:07:58]

A revert of this CL has been created in
https://codereview.chromium.org/357693004/ by miguelg@chromium.org.

The reason for reverting is: It seems to be checking code that is not part of
the diff. 

I also think there are some legitimate cases for not reached in the middle of a
block but that is something that can be discussed on the bug.

Filed https://code.google.com/p/chromium/issues/detail?id=388731 to track..