Skip to content

Commit

Permalink
Fix false-positive in unique_ptr presubmit check with mismatched <>
Browse files Browse the repository at this point in the history
Presubmit was preventing upload of crrev.com/c/5534806 due to changed
line:
` std::unique_ptr<WebAppInstallInfo>>(`
being interpreted as a call to the unique_ptr constructor. IIUC this was
because the check only looks at the changed lines and missed the extra
">" indicating this was really part of a larger template expression.

Added a condition that suppresses the check when selected brackets are
mismatching (ie. number of < and > are not equal). Added tests for these
cases.

Change-Id: I260b7826eb1f55764a8a93b3402888afa5032eed
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5538411
Reviewed-by: Dirk Pranke <[email protected]>
Auto-Submit: Glen Robertson <[email protected]>
Commit-Queue: Glen Robertson <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1301700}
  • Loading branch information
Glen Robertson authored and Chromium LUCI CQ committed May 16, 2024
1 parent b79c4fb commit 9142ffd
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 4 deletions.
10 changes: 7 additions & 3 deletions PRESUBMIT.py
Original file line number Diff line number Diff line change
Expand Up @@ -3548,8 +3548,9 @@ def CheckForAnonymousVariables(input_api, output_api):

def CheckUniquePtrOnUpload(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):
# and U, or is invalid due to mismatched angle bracket pairs. Assumes that
# |template_str| is already in the form <...>.
def HasMoreThanOneArgOrInvalid(template_str):
# Level of <...> nesting.
nesting = 0
for c in template_str:
Expand All @@ -3559,6 +3560,9 @@ def HasMoreThanOneArg(template_str):
nesting -= 1
elif c == ',' and nesting == 1:
return True
if nesting != 0:
# Invalid.
return True
return False

file_inclusion_pattern = [r'.+%s' % _IMPLEMENTATION_EXTENSIONS]
Expand Down Expand Up @@ -3615,7 +3619,7 @@ def HasMoreThanOneArg(template_str):
# bar = std::unique_ptr<T, U>(foo);
local_path = f.LocalPath()
return_construct_result = return_construct_pattern.search(line)
if return_construct_result and not HasMoreThanOneArg(
if return_construct_result and not HasMoreThanOneArgOrInvalid(
return_construct_result.group('template_arg')):
problems_constructor.append(
'%s:%d\n %s' % (local_path, line_number, line.strip()))
Expand Down
5 changes: 4 additions & 1 deletion PRESUBMIT_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -3164,6 +3164,9 @@ def testFalsePositives(self):
]),
MockFile('dir/nested.cc', ['set<std::unique_ptr<T>>();']),
MockFile('dir/nested2.cc', ['map<U, std::unique_ptr<T>>();']),
# Changed line is inside a multiline template block.
MockFile('dir/template.cc', [' std::unique_ptr<T>>(']),
MockFile('dir/template2.cc', [' std::unique_ptr<T>>()']),

# Two-argument invocation of std::unique_ptr is exempt because there is
# no equivalent using std::make_unique.
Expand All @@ -3172,7 +3175,7 @@ def testFalsePositives(self):
]

results = PRESUBMIT.CheckUniquePtrOnUpload(mock_input_api, MockOutputApi())
self.assertEqual(0, len(results))
self.assertEqual([], results)

class CheckNoDirectIncludesHeadersWhichRedefineStrCat(unittest.TestCase):
def testBlocksDirectIncludes(self):
Expand Down

0 comments on commit 9142ffd

Please sign in to comment.