Make presubmit check that #if or #ifdef does not come before includes

PRESUBMIT.py

 # Copyright (c) 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 Skia.
 
 See http://dev.chromium.org/developers/how-tos/depottools/presubmit-scripts
 for more details about the presubmit API built into gcl.
 """
@@ skipping 59 matching lines @@
   for affected_file in input_api.AffectedSourceFiles(None):
     affected_file_path = affected_file.LocalPath()
     if affected_file_path.endswith('.py'):
       affected_python_files.append(affected_file_path)
   return input_api.canned_checks.RunPylint(
       input_api, output_api,
       disabled_warnings=pylint_disabled_warnings,
       white_list=affected_python_files)
 
 
+def _IfDefChecks(input_api, output_api):
+  """Ensures if/ifdef are not before includes. See skbug/3362 for details."""
+  copyright_start_pattern = re.compile('\/\*\w*\n')
+  copyright_middle_pattern = re.compile(' \*.*\n')
+  copyright_end_pattern = re.compile(' \*\/.*\n')

[borenet, 2015/03/12 12:47:48]

Suggest using '^\/\*w*$' etc

[rmistry, 2015/03/12 13:13:59]

Yes, I forgot to do this. Done.

+  def is_in_copyright_block(line):
+    return (copyright_start_pattern.match(line) or
+            copyright_middle_pattern.match(line) or
+            copyright_end_pattern.match(line))
+
+  empty_line_pattern = re.compile('\w*\n\w*')

[borenet, 2015/03/12 12:47:48]

Shouldn't this be '^$' ?  Why match characters in a line that's supposed to be
empty?

[rmistry, 2015/03/12 13:13:59]

This is to treat lines that have only whitespaces in it as empty lines. If there
is a '  ' line and empty_line_pattern does not catch it then it will be treated
as the first real line and will be tested for '#if' and '#ifdef'.

+  def is_empty_line(line):
+    return empty_line_pattern.match(line)
+
+  failing_files = []
+  for affected_file in input_api.AffectedSourceFiles(None):
+    affected_file_path = affected_file.LocalPath()
+    if affected_file_path.endswith('.cpp') or affected_file_path.endswith('.h'):
+      f = open(affected_file_path)
+      for line in f.xreadlines():
+        if is_in_copyright_block(line) or is_empty_line(line):
+          continue
+        # The below will be the first real line after the copyright block and
+        # newlines.
+        if line.startswith('#if 0 '):
+          pass

[borenet, 2015/03/12 12:47:48]

Why not break here?

[rmistry, 2015/03/12 13:13:59]

Either way works. I prefer having less places to exit whenever possible.

+        elif line.startswith('#if ') or line.startswith('#ifdef '):
+          failing_files.append(affected_file_path)
+        break
+
+  results = []
+  if failing_files:
+    results.append(
+        output_api.PresubmitError(
+            'The following files have #if or #ifdef before includes:\n%s\n\n'
+            'See skbug.com/3362 for why this should be fixed.' %
+                '\n'.join(failing_files)))
+  return results
+
+
 def _CommonChecks(input_api, output_api):
   """Presubmit checks common to upload and commit."""
   results = []
   sources = lambda x: (x.LocalPath().endswith('.h') or
                        x.LocalPath().endswith('.gypi') or
                        x.LocalPath().endswith('.gyp') or
                        x.LocalPath().endswith('.py') or
                        x.LocalPath().endswith('.sh') or
                        x.LocalPath().endswith('.cpp'))
   results.extend(
       _CheckChangeHasEol(
           input_api, output_api, source_file_filter=sources))
   results.extend(_PythonChecks(input_api, output_api))
+  results.extend(_IfDefChecks(input_api, output_api))
   return results
 
 
 def CheckChangeOnUpload(input_api, output_api):
   """Presubmit checks for the change on upload.
 
   The following are the presubmit checks:
   * Check change has one and only one EOL.
   """
   results = []
@@ skipping 230 matching lines @@
     state and an error if it is in 'Closed' state.
   """
   results = []
   results.extend(_CommonChecks(input_api, output_api))
   results.extend(
       _CheckTreeStatus(input_api, output_api, json_url=(
           SKIA_TREE_STATUS_URL + '/banner-status?format=json')))
   results.extend(_CheckLGTMsForPublicAPI(input_api, output_api))
   results.extend(_CheckOwnerIsInAuthorsFile(input_api, output_api))
   return results