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

media/PRESUBMIT.py

Index: media/PRESUBMIT.py
diff --git a/media/PRESUBMIT.py b/media/PRESUBMIT.py
index feba2f719a5c6fe3c35fdb514af8fb5ef61cbbd4..8f654c481f9a51cb1b821df6155b7f6f2b9d907f 100644
--- a/media/PRESUBMIT.py
+++ b/media/PRESUBMIT.py
@@ -8,6 +8,9 @@ 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(
@@ -158,11 +161,33 @@ def _CheckForHistogramOffByOne(input_api, output_api):
 
   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