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..6d48a6f3e89eb3d70ee73409918b48a4cdc4efa0 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,34 @@ def _CheckPatchFiles(input_api, output_api):
     return []
 
 
+def _CheckForInvalidOSMacrosInFile(input_api, f):
+  """Check for sensible looking, totally invalid OS macros."""
+  starts_with = input_api.re.compile(r'#(?:else|(?:el|end)?if(?:n?def)?)')

[M-A Ruel, 2013/03/22 12:41:15]

I agree with Scott the regexp is hard to read and ... it's wrong. Both following
lines are valid preprocessor conditions:
  #if FOO
#  if FOO

Note that you do a match but do not use ^. In practice it's fine so you match
the first line, even if it's not obvious, but you don't match the second one. So
for example, it'd match lines like:
// There's #ifdef here but it's a comment

So the name starts_with is confusing.

[Dan Beam, 2013/03/22 18:29:02]

This will never be "right" unless it's a real C compiler.  See numerous other
places in this file that are partially parsing C.

Do you want me to update with \s*#\s*?
so you'd rather me add a ^ to the regexp and/or change to input_api.re.search()?

[Dan Beam, 2013/03/22 18:32:22]

shess@ also recommended only looking defined(OS_NAME), would that be better?

[M-A Ruel, 2013/03/22 18:33:11]

That's true, I just want to make sure the _intent_ is explicit and readable. And
on that point, the intent is not well communicated because the variable name
doesn't match what the regexp does.

I don't have much opinion otherwise.

[Dan Beam, 2013/03/22 18:48:02]

Fixed this and added test cases.

+  os_macro = input_api.re.compile(r'\b(OS_[A-Z]+)\b')
+  results = []
+  for lnum, line in f.ChangedContents():
+    if starts_with.match(line):
+      for match in os_macro.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', '.js', '.html', '.css')):
+      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))