Skip to content

Commit

Permalink
Revert of Add PRESUBMIT.py warning for contradictory NOTREACHED() use. (
Browse files Browse the repository at this point in the history
https://codereview.chromium.org/344563003/)

Reason for revert:
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.

Original issue's description:
> 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

TBR=maruel@chromium.org,tnagel@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=none

Review URL: https://codereview.chromium.org/357693004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@279710 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
miguelg@chromium.org committed Jun 25, 2014
1 parent 6131fe6 commit 5e4dbc7
Show file tree
Hide file tree
Showing 2 changed files with 0 additions and 135 deletions.
89 changes: 0 additions & 89 deletions PRESUBMIT.py
Original file line number Diff line number Diff line change
Expand Up @@ -1229,94 +1229,6 @@ def _CheckNoDeprecatedCSS(input_api, output_api):
(fpath.LocalPath(), line_num, deprecated_value, value)))
return results


def _StripCommentsAndStrings(input_api, s):
"""Remove comments, replace string literals by a single token. Requires that
input data is formatted with unix-style line ends."""

s = input_api.re.sub(r'\\\n', r'', s) # Continue lines ending in backslash.

out = ''
i = 0
while i < len(s):
c = s[i]

if c == '/':
mo = input_api.re.match(r'//.*', s[i:])
if mo:
i += len(mo.group(0))
continue
mo = input_api.re.match(r'/\*.*?\*/', s[i:], input_api.re.DOTALL)
if mo:
i += len(mo.group(0))
continue

if c == "'":
mo = input_api.re.match(r"'((\\\\)|(\\')|[^']+?)'", s[i:])
if not mo:
raise Exception('bad char: ' + s[i:])
i += len(mo.group(0))
out += ' CHAR_LITERAL '
continue

if c == '"':
mo = input_api.re.match(r'".*?(?<!\\)(\\\\)*"', s[i:])
if not mo:
raise Exception('bad string: ' + s[i:])
i += len(mo.group(0))
out += ' STRING_LITERAL '
continue

out += c
i += 1

return out


def _CheckContradictoryNotreachedUse(input_api, output_api):
file_inclusion_pattern = (
r".+\.c$", r".+\.cc$", r".+\.cpp$", r".+\.h$", r".+\.hpp$", r".+\.inl$",
r".+\.m$", r".+\.mm$" )
black_list = (_EXCLUDED_PATHS + input_api.DEFAULT_BLACK_LIST)
file_filter = lambda f: input_api.FilterSourceFile(
f, white_list=file_inclusion_pattern, black_list=black_list)
results = []
for fpath in input_api.AffectedFiles(file_filter=file_filter):
results.extend(_CheckContradictoryNotreachedUseInFile(input_api, fpath))
return [output_api.PresubmitPromptWarning(r) for r in results]


def _CheckContradictoryNotreachedUseInFile(input_api, f):
style_url = (
'http://chromium.org/developers/coding-style'
'#TOC-CHECK-DCHECK-and-NOTREACHED-')
contents = f.NewContents()
text = ''.join(line + '\n' for line in f.NewContents())
text = _StripCommentsAndStrings(input_api, text)

results = []
while True:
# Capture text between NOTREACHED(); and the next closing brace or "break".
mo = input_api.re.search(
r'[ \t]*NOTREACHED\(\s*\).*?;(?P<between>.*?)((\bbreak\b)|})',
text, input_api.re.DOTALL)
# TODO(tnagel): Catch loops inside which NOTREACHED() is followed by break.
if not mo:
break
text = text[mo.end():]
if input_api.re.match(r'[\s;]*$', mo.group('between'), input_api.re.DOTALL):
continue
excerpt = mo.group(0).rstrip()
if len(excerpt) > 100:
excerpt = excerpt[:100] + ' \u2026' # ellipsis
results.append(
'%s: NOTREACHED() may only be used at end-of-block '
'but is followed by code.\n%s\n'
'Offending section (comments/strings possibly stripped):\n%s'
% (f, style_url, excerpt))
return results


def _CommonChecks(input_api, output_api):
"""Checks common to both upload and commit."""
results = []
Expand Down Expand Up @@ -1354,7 +1266,6 @@ def _CommonChecks(input_api, output_api):
results.extend(_CheckUserActionUpdate(input_api, output_api))
results.extend(_CheckNoDeprecatedCSS(input_api, output_api))
results.extend(_CheckParseErrors(input_api, output_api))
results.extend(_CheckContradictoryNotreachedUse(input_api, output_api))

if any('PRESUBMIT.py' == f.LocalPath() for f in input_api.AffectedFiles()):
results.extend(input_api.canned_checks.RunUnitTestsInDirectory(
Expand Down
46 changes: 0 additions & 46 deletions PRESUBMIT_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -413,52 +413,6 @@ def testValidOSMacroNames(self):
self.assertEqual(0, len(errors))


class CheckContradictoryNotreachedUseTest(unittest.TestCase):
def testValid(self):
lines = ['{',
' // NOTREACHED();',
' /* NOTREACHED(); */',
" char a = '\\\\', b = '\\0', c = '\\'';",
' char d[] = "NOTREACHED();";',
' NOTREACHED(); // blah',
' /* comment */',
' // line continuation \\',
' still inside comment',
' // comment followed by empty line',
'',
' ;; ; ;;;',
'}',
'switch (i) {',
' case 7: NOTREACHED(); break;',
'}']
output = PRESUBMIT._CheckContradictoryNotreachedUseInFile(
MockInputApi(), MockFile('some/path/foo_platform.cc', lines))
self.assertEqual(0, len(output))

def testInvalid(self):
lines = ['{',
' NOTREACHED();',
' return;',
'}',
'{',
' NOTREACHED();',
' /* */',
' return;',
' /* */',
'}',
'{',
' NOTREACHED();',
' // trailing space, not a line continuation \\ ',
' return;',
'}',
'switch (i) {',
' case 7: NOTREACHED(); some_thing(); break;',
'}']
output = PRESUBMIT._CheckContradictoryNotreachedUseInFile(
MockInputApi(), MockFile('some/path/foo_platform.cc', lines))
self.assertEqual(4, len(output))


class CheckAddedDepsHaveTetsApprovalsTest(unittest.TestCase):
def testFilesToCheckForIncomingDeps(self):
changed_lines = [
Expand Down

0 comments on commit 5e4dbc7

Please sign in to comment.