Adding _Check* function for invalid OS_MACROs in src/PRESUBMIT.py

PRESUBMIT.py

Index: PRESUBMIT.py
diff --git a/PRESUBMIT.py b/PRESUBMIT.py
index a5ea9e45b442405f8db7a086ab7218c239effde2..4b72ebcb7c3d7067fd13d08883a0db2825ef9400 100644
--- a/PRESUBMIT.py
+++ b/PRESUBMIT.py
@@ -165,6 +165,25 @@ _BANNED_CPP_FUNCTIONS = (
 )
 
 
+_VALID_OS_MACROS = (
+    # Please keep sorted.
+    'OS_ANDROID',
+    'OS_BSD',
+    'OS_CAT',       # For testing.
+    'OS_CHROMEOS',
+    'OS_FREEBSD',
+    'OS_IOS',
+    'OS_LINUX',
+    'OS_MACOSX',
+    'OS_NACL',
+    'OS_OPENBSD',
+    'OS_POSIX',
+    'OS_SOLARIS',
+    'OS_SUN',       # Not in build/build_config.h but in skia.
+    'OS_WIN',
+)
+
+
 def _CheckNoProductionCodeUsingTestOnlyFunctions(input_api, output_api):
   """Attempts to prevent use of functions intended only for testing in
   non-testing code. For now this is just a best-effort implementation
@@ -714,6 +733,7 @@ def _CommonChecks(input_api, output_api):
   results.extend(_CheckPatchFiles(input_api, output_api))
   results.extend(_CheckHardcodedGoogleHostsInLowerLayers(input_api, output_api))
   results.extend(_CheckNoAbbreviationInPngFileName(input_api, output_api))
+  results.extend(_CheckForInvalidOSMacros(input_api, output_api))
 
   if any('PRESUBMIT.py' == f.LocalPath() for f in input_api.AffectedFiles()):
     results.extend(input_api.canned_checks.RunUnitTestsInDirectory(
@@ -806,6 +826,32 @@ def _CheckPatchFiles(input_api, output_api):
     return []
 
 
+def _CheckForInvalidOSMacrosInFile(input_api, f):
+  """Check for sensible looking, totally invalid OS macros."""
+  results = []
+  pattern = input_api.re.compile(r'\b(OS_[A-Z]{3,})\b')

[Scott Hess - ex-Googler, 2013/03/21 04:20:00]

Have you run this over the existing codebase?  I haven't actually checked for
cases where we have OS_SOMETHING not inside of defined(OS_SOMETHING).  If it's
not in a defined() clause, then it's completely incorrect anyhow, so I'm not
sure whether I care about warning on those cases, but I do think it's worthwhile
to not have people routinely overriding the presubmit because of false
positives.

[Dan Beam, 2013/03/21 04:29:09]

I think people putting OS_BLAH in comments is equally bad at propagating the
wrong names as

  #if defined(OS_BLAH)

which is why I'm intentionally not looking for

  ^#(?:el|end)?if(?:n?def)?

And no, I haven't run it on the whole codebase (yet).

[Scott Hess - ex-Googler, 2013/03/21 05:03:51]

I can see both ways, but if the comment doesn't contain "defined(OS_BLAH)", then
they cannot easily copy/paste it to propagate the problem.  And even if they
copy/paste and add defined(), the presubmit will catch them on it.

+  for lnum, line in f.ChangedContents():
+    for match in pattern.finditer(line):
+      if not match.group(1) in _VALID_OS_MACROS:
+        results.append('    %s:%d %s' % (f.LocalPath(), lnum, match.group(1)))
+  return results
+
+
+def _CheckForInvalidOSMacros(input_api, output_api):
+  """Check all affected files for invalid OS macros."""
+  bad_macros = []
+  for f in input_api.AffectedFiles():
+    if not f.LocalPath().endswith('.py'):
+      bad_macros.extend(_CheckForInvalidOSMacrosInFile(input_api, f))
+
+  if not bad_macros:
+    return []
+
+  return [output_api.PresubmitError(
+      'Possibly invalid OS macros found. Please add your macro to\n'
+      'src/PRESUBMIT.py or update your code.', bad_macros)]
+
+
 def CheckChangeOnUpload(input_api, output_api):
   results = []
   results.extend(_CommonChecks(input_api, output_api))