Skip to content

Commit

Permalink
Allow std::unique_ptr<T, D>(...) in presubmit
Browse files Browse the repository at this point in the history
The _CheckUniquePtr presubmit check has the following false-positive:
it flags
  auto p = std::unique_ptr<T, D>(new T(), D());
as needing a rewrite using std::make_unique.

However, there seems to be no way to pass the second template argument
to std::make_unique, so cases like this need to be exempt from the
presubmit check.

Note that the check still needs to capture cases with

count the nesting of the angle brackets. Hence, this check cannot be
captured by a regular expression and needs just a counting loop.

std: :unique_ptr<T<U, V>>, so when looking for the comma, one has to
Bug: 867823
Change-Id: Ie21067fa83aae17dd6701b0f69b902605c797e67
Reviewed-on: https://chromium-review.googlesource.com/1193945
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587463}
  • Loading branch information
Vaclav Brozek authored and Commit Bot committed Aug 30, 2018
1 parent be0152c commit b7fadb6
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 2 deletions.
25 changes: 24 additions & 1 deletion PRESUBMIT.py
Original file line number Diff line number Diff line change
Expand Up @@ -1681,6 +1681,20 @@ def _CheckForAnonymousVariables(input_api, output_api):


def _CheckUniquePtr(input_api, output_api):
# Returns whether |template_str| is of the form <T, U...> for some types T
# and U. Assumes that |template_str| is already in the form <...>.
def HasMoreThanOneArg(template_str):
# Level of <...> nesting.
nesting = 0
for c in template_str:
if c == '<':
nesting += 1
elif c == '>':
nesting -= 1
elif c == ',' and nesting == 1:
return True
return False

file_inclusion_pattern = [r'.+%s' % _IMPLEMENTATION_EXTENSIONS]
sources = lambda affected_file: input_api.FilterSourceFile(
affected_file,
Expand Down Expand Up @@ -1716,10 +1730,13 @@ def _CheckUniquePtr(input_api, output_api):
# Suffix saying that what follows are call parentheses with a non-empty list
# of arguments.
nonempty_arg_list_pattern = r'\(([^)]|$)'
# Put the template argument into a capture group for deeper examination later.
return_construct_pattern = input_api.re.compile(
start_of_expr_pattern
+ r'std::unique_ptr'
+ '(?P<template_arg>'
+ template_arg_no_array_pattern
+ ')'
+ nonempty_arg_list_pattern)

problems_constructor = []
Expand All @@ -1732,8 +1749,14 @@ def _CheckUniquePtr(input_api, output_api):
# But allow:
# return std::unique_ptr<T[]>(foo);
# bar = std::unique_ptr<T[]>(foo);
# And also allow cases when the second template argument is present. Those
# cases cannot be handled by std::make_unique:
# return std::unique_ptr<T, U>(foo);
# bar = std::unique_ptr<T, U>(foo);
local_path = f.LocalPath()
if return_construct_pattern.search(line):
return_construct_result = return_construct_pattern.search(line)
if return_construct_result and not HasMoreThanOneArg(
return_construct_result.group('template_arg')):
problems_constructor.append(
'%s:%d\n %s' % (local_path, line_number, line.strip()))
# Disallow:
Expand Down
10 changes: 9 additions & 1 deletion PRESUBMIT_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -1820,17 +1820,20 @@ def testTruePositivesConstructor(self):
'bar = std::unique_ptr<T>(',
' fooVeryVeryVeryLongStillGoingWellThisWillTakeAWhileFinallyThere);'
]),
MockFile('dir/multi_arg.cc', [
'auto p = std::unique_ptr<std::pair<T, D>>(new std::pair(T, D));']),
]

results = PRESUBMIT._CheckUniquePtr(mock_input_api, MockOutputApi())
self.assertEqual(1, len(results))
self.assertTrue('std::make_unique' in results[0].message)
self.assertEqual(5, len(results[0].items))
self.assertEqual(6, len(results[0].items))
self.assertTrue('foo.cc' in results[0].items[0])
self.assertTrue('bar.mm' in results[0].items[1])
self.assertTrue('mult.cc' in results[0].items[2])
self.assertTrue('mult2.cc' in results[0].items[3])
self.assertTrue('mult3.cc' in results[0].items[4])
self.assertTrue('multi_arg.cc' in results[0].items[5])

def testFalsePositives(self):
mock_input_api = MockInputApi()
Expand All @@ -1846,6 +1849,11 @@ def testFalsePositives(self):
]),
MockFile('dir/nested.cc', ['set<std::unique_ptr<T>>();']),
MockFile('dir/nested2.cc', ['map<U, std::unique_ptr<T>>();']),

# Two-argument invocation of std::unique_ptr is exempt because there is
# no equivalent using std::make_unique.
MockFile('dir/multi_arg.cc', [
'auto p = std::unique_ptr<T, D>(new T(), D());']),
]

results = PRESUBMIT._CheckUniquePtr(mock_input_api, MockOutputApi())
Expand Down

0 comments on commit b7fadb6

Please sign in to comment.