Reduce _CheckUniquePtr spam

Currently, _CheckUniquePtr PRESUBMIT check reports each failing line as
a separate error. This repeats the error explanation and thus clutters
the output necessarily.

This CL makes _CheckUniquePtr collect all occurences of the two issues
it checks for (direct use of unique_ptr constructor and replaceability
with nullptr) and group them under a separate single error, one for each
of the both types of check.

It also adds the failing line into the output, to make it easier to
understand the issue already from the presubmit logs.

This follows what is done for other checks, e.g., _CheckNoPragmaOnce).

Bug: 827961
Change-Id: Ic7d60a05b6f96da741f1401422f4a1690bb6e279
Reviewed-on: https://chromium-review.googlesource.com/990132
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548081}

PRESUBMIT.py

@@ -1589,7 +1589,8 @@ def _CheckUniquePtr(input_api, output_api):
       r'(=|\breturn|^)\s*std::unique_ptr<.*?(?<!])>\(([^)]|$)')
   null_construct_pattern = input_api.re.compile(
       r'\b(?<!<)std::unique_ptr<[^>]*>([^(<]*>)?\(\)')
-  errors = []
+  problems_constructor = []
+  problems_nullptr = []
   for f in input_api.AffectedSourceFiles(sources):
     for line_number, line in f.ChangedContents():
       # Disallow:
@@ -1598,17 +1599,26 @@ def _CheckUniquePtr(input_api, output_api):
       # But allow:
       # return std::unique_ptr<T[]>(foo);
       # bar = std::unique_ptr<T[]>(foo);
+      local_path = f.LocalPath()
       if return_construct_pattern.search(line):
-        errors.append(output_api.PresubmitError(
-          ('%s:%d uses explicit std::unique_ptr constructor. ' +
-           'Use std::make_unique<T>() instead.') %
-          (f.LocalPath(), line_number)))
+        problems_constructor.append(
+          '%s:%d\n    %s' % (local_path, line_number, line.strip()))
       # Disallow:
       # std::unique_ptr<T>()
       if null_construct_pattern.search(line):
+        problems_nullptr.append(
+          '%s:%d\n    %s' % (local_path, line_number, line.strip()))
+
+  errors = []
+  if problems_constructor:
+    errors.append(output_api.PresubmitError(
+        'The following files use std::unique_ptr<T>(). Use nullptr instead.',
+        problems_constructor))
+  if problems_nullptr:
     errors.append(output_api.PresubmitError(
-          '%s:%d uses std::unique_ptr<T>(). Use nullptr instead.' %
-          (f.LocalPath(), line_number)))
+        'The following files use explicit std::unique_ptr constructor.'
+        'Use std::make_unique<T>() instead.',
+        problems_nullptr))
   return errors
 
 

PRESUBMIT_test.py

@@ -1639,13 +1639,24 @@ class NoProductionJavaCodeUsingTestOnlyFunctions(unittest.TestCase):
 
 
 class CheckUniquePtr(unittest.TestCase):
-  def testTruePositives(self):
+  def testTruePositivesNullptr(self):
     mock_input_api = MockInputApi()
     mock_input_api.files = [
-      MockFile('dir/java/src/foo.cc', ['return std::unique_ptr<T>(foo);']),
-      MockFile('dir/java/src/bar.mm', ['bar = std::unique_ptr<T>(foo)']),
       MockFile('dir/java/src/baz.cc', ['std::unique_ptr<T>()']),
       MockFile('dir/java/src/baz-p.cc', ['std::unique_ptr<T<P>>()']),
+    ]
+
+    results = PRESUBMIT._CheckUniquePtr(mock_input_api, MockOutputApi())
+    self.assertEqual(1, len(results))
+    self.assertEqual(2, len(results[0].items))
+    self.assertTrue('baz.cc' in results[0].items[0])
+    self.assertTrue('baz-p.cc' in results[0].items[1])
+
+  def testTruePositivesConstructor(self):
+    mock_input_api = MockInputApi()
+    mock_input_api.files = [
+      MockFile('dir/java/src/foo.cc', ['return std::unique_ptr<T>(foo);']),
+      MockFile('dir/java/src/bar.mm', ['bar = std::unique_ptr<T>(foo)']),
       MockFile('dir/java/src/mult.cc', [
         'return',
         '    std::unique_ptr<T>(barVeryVeryLongFooSoThatItWouldNotFitAbove);'
@@ -1661,16 +1672,13 @@ class CheckUniquePtr(unittest.TestCase):
     ]
 
     results = PRESUBMIT._CheckUniquePtr(mock_input_api, MockOutputApi())
-    # TODO(crbug.com/827961) Make the check return just one result, listing all
-    # affected files in it.
-    self.assertEqual(7, len(results))
-    self.assertTrue('foo.cc' in results[0].message)
-    self.assertTrue('bar.mm' in results[1].message)
-    self.assertTrue('baz.cc' in results[2].message)
-    self.assertTrue('baz-p.cc' in results[3].message)
-    self.assertTrue('mult.cc' in results[4].message)
-    self.assertTrue('mult2.cc' in results[5].message)
-    self.assertTrue('mult3.cc' in results[6].message)
+    self.assertEqual(1, len(results))
+    self.assertEqual(5, 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])
 
   def testFalsePositives(self):
     mock_input_api = MockInputApi()