media/PRESUBMIT.py: adding CheckPassByValue for base::Time and derived classes

media/PRESUBMIT.py

 # Copyright 2013 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Top-level presubmit script for Chromium media component.
 
 See http://dev.chromium.org/developers/how-tos/depottools/presubmit-scripts
 for more details about the presubmit API built into depot_tools.
 """
 
+import re
+import string
+
 def _FilterFile(affected_file):
   """Return true if the file could contain code requiring a presubmit check."""
   return affected_file.LocalPath().endswith(
       ('.h', '.cc', '.cpp', '.cxx', '.mm'))
 
 
 def _CheckForUseOfWrongClock(input_api, output_api):
   """Make sure new lines of media code don't use a clock susceptible to skew."""
 
   # Regular expression that should detect any explicit references to the
@@ skipping 130 matching lines @@
     return [output_api.PresubmitError(
       'UMA_HISTOGRAM_ENUMERATION reports in src/media/ are expected to adhere\n'
       'to the following guidelines:\n'
       ' - The max value (3rd argument) should be an enum value equal to the\n'
       '   last valid value, e.g. FOO_MAX = LAST_VALID_FOO.\n'
       ' - 1 must be added to that max value.\n'
       'Contact rileya@chromium.org if you have questions.' , problems)]
 
   return []
 
+def _CheckPassByValue(input_api,

[sandersd (OOO until July 31), 2015/10/16 00:54:31]

No need to wrap arguments.

[mcasas, 2015/10/16 18:00:11]

Done.

+                      output_api):
+  local_errors = []
+
+  # Well-defined simple classes containing only <= 4 ints, or <= 2 floats.
+  pass_by_value_types = ['base::Time',
+                         'base::TimeTicks',
+                         ]
+
+  for f in input_api.AffectedSourceFiles(_FilterFile):
+    contents = input_api.ReadFile(f, 'rb')
+    match = re.search(
+      r'\bconst +' + '(?P<type>(%s))&' %

[sandersd (OOO until July 31), 2015/10/16 00:54:31]

This regex should be precompiled, eg:

PASS_BY_VALUE_RE = re.compile(r"...")


def _CheckPassByValue(...):
  match = PASS_BY_VALUE_RE.search(contents)

[mcasas, 2015/10/16 18:00:11]

Done.

+        string.join(pass_by_value_types, '|'),

[sandersd (OOO until July 31), 2015/10/16 00:54:31]

Instead of building 'const foo&|cost bar&', it would make more sense to build
'cost (foo|bar)&'. This would remove the need for a group name and make it much
easier to read.

[mcasas, 2015/10/16 18:00:11]

Isn't it what's done? Regexp is

r'... <type>(%s))&' % string.join(pass_by_value_types, '|')

expands to:

r'... <type>(%s))&' % 'base::Time|base::TimeTicks'

which expands to:

r'... <type>(base::Time|base::TimeTicks))&'

[sandersd (OOO until July 31), 2015/10/16 18:27:12]

Hmm, you're correct, but that means there is no need for naming the capture
group in the current form. It's syntactic noise I'd rather was removed.

[mcasas, 2015/10/16 21:40:42]

Done.

+      contents)
+    if match:

[sandersd (OOO until July 31), 2015/10/16 00:54:31]

Note that there is also .findall(), which iterates over all non-overlapping
matches. That may be better if the goal is to list all instances to be fixed.

[mcasas, 2015/10/16 18:00:11]

My first goal was to stick as much as possible to the 
cc/PRESUBMIT.py version. It's true that this code 
would only list the file while not indicating the line
for the syntax bug and/or if there's one or multiple 
hits. WDYT? Should I make this code more 
sophisticated? Or does it help enough 'as is'?

[sandersd (OOO until July 31), 2015/10/16 18:27:12]

Probably best to list them all, it's a trivial change.

[mcasas, 2015/10/16 21:40:42]

Not that trivial since find_all() would not give the line
number, and we would like to give that information,
correct? I rewrote the whole routine to look more 
like the neighbours.

+      local_errors.append(output_api.PresubmitError(
+        '%s passes %s by const ref instead of by value.' %
+        (f.LocalPath(), match.group('type'))))
+  return local_errors
+
 
 def _CheckChange(input_api, output_api):
   results = []
   results.extend(_CheckForUseOfWrongClock(input_api, output_api))
   results.extend(_CheckForMessageLoopProxy(input_api, output_api))
+  results.extend(_CheckPassByValue(input_api, output_api))
   results.extend(_CheckForHistogramOffByOne(input_api, output_api))
   return results
 
 
 def CheckChangeOnUpload(input_api, output_api):
   return _CheckChange(input_api, output_api)
 
 
 def CheckChangeOnCommit(input_api, output_api):
   return _CheckChange(input_api, output_api)