PRESUBMIT #include check enhancements.

PRESUBMIT #include check enhancements.

1) Handle #define and #undef the same way as #if etc. See e.g.,
https://codereview.chromium.org/11363222/diff/10015/chrome/browser/history/history_unittest.cc
line 545.

2) Also headers can have the special first include, not only sources. See e.g.,
net/disk_cache/storage_block-inl.h.

NOTRY=true
BUG=NONE


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171511

[marja, 2012-12-06 14:32:46]

ptal.

The diff is wonky, it didn't really diff nicely because there was one part that
said
        line_num -= 1
        break
and another which said
        line_num -= 1
      break

And then I moved everything 2 spaces back, so the diff looks a bit weird.

[M-A Ruel, 2012-12-06 15:16:31]

https://codereview.chromium.org/11441035/diff/1/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/11441035/diff/1/PRESUBMIT.py#newcode617
PRESUBMIT.py:617: warnings.extend(_CheckIncludeOrderInFile(input_api, f,
changed_linenums))
Do you really want to run it over .java and .py files?

[marja, 2012-12-06 15:23:08]

thanks for comments!

https://codereview.chromium.org/11441035/diff/1/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/11441035/diff/1/PRESUBMIT.py#newcode617
PRESUBMIT.py:617: warnings.extend(_CheckIncludeOrderInFile(input_api, f,
changed_linenums))
On 2012/12/06 15:16:32, Marc-Antoine Ruel wrote:
> Do you really want to run it over .java and .py files?

-> fixed

[M-A Ruel, 2012-12-06 15:29:27]

https://codereview.chromium.org/11441035/diff/5001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/11441035/diff/5001/PRESUBMIT.py#newcode598
PRESUBMIT.py:598: changed_linenums))
Note that you are never doing anything with changed_linenums, is that expected?

https://codereview.chromium.org/11441035/diff/5001/PRESUBMIT.py#newcode616
PRESUBMIT.py:616: if f.LocalPath().endswith('.cc') or
f.LocalPath().endswith('.h'):
if f.LocalPath().endswith(('.cc', '.h')):

https://codereview.chromium.org/11441035/diff/5001/PRESUBMIT.py#newcode617
PRESUBMIT.py:617: changed_linenums = set([line_num for line_num, _ in
f.ChangedContents()])
changed_linenums = set(line_num for line_num, _ in f.ChangedContents())

but why put it in a set exactly? A list would be fine and faster.

[marja, 2012-12-06 16:02:52]

Thanks for comments again. I also added a test.

https://codereview.chromium.org/11441035/diff/5001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/11441035/diff/5001/PRESUBMIT.py#newcode598
PRESUBMIT.py:598: changed_linenums))
On 2012/12/06 15:29:27, Marc-Antoine Ruel wrote:
> Note that you are never doing anything with changed_linenums, is that
expected?

Line 540 uses the changed_linenums (to not whine about #includes which were
broken before the changes). testOrderAlreadyWrong tests that this is done
properly.

https://codereview.chromium.org/11441035/diff/5001/PRESUBMIT.py#newcode616
PRESUBMIT.py:616: if f.LocalPath().endswith('.cc') or
f.LocalPath().endswith('.h'):
On 2012/12/06 15:29:27, Marc-Antoine Ruel wrote:
> if f.LocalPath().endswith(('.cc', '.h')):

Done.

https://codereview.chromium.org/11441035/diff/5001/PRESUBMIT.py#newcode617
PRESUBMIT.py:617: changed_linenums = set([line_num for line_num, _ in
f.ChangedContents()])
On 2012/12/06 15:29:27, Marc-Antoine Ruel wrote:
> changed_linenums = set(line_num for line_num, _ in f.ChangedContents())
> 
> but why put it in a set exactly? A list would be fine and faster.

I made the changed_linenums a set, since the check is "are the 2 lines between
which the error is included in the changed lines", so set should be better for
that than a list, no?

[M-A Ruel, 2012-12-06 16:11:26]

https://codereview.chromium.org/11441035/diff/5001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/11441035/diff/5001/PRESUBMIT.py#newcode617
PRESUBMIT.py:617: changed_linenums = set([line_num for line_num, _ in
f.ChangedContents()])
On 2012/12/06 16:02:53, marja wrote:
> On 2012/12/06 15:29:27, Marc-Antoine Ruel wrote:
> > changed_linenums = set(line_num for line_num, _ in f.ChangedContents())
> > 
> > but why put it in a set exactly? A list would be fine and faster.
> 
> I made the changed_linenums a set, since the check is "are the 2 lines between
> which the error is included in the changed lines", so set should be better for
> that than a list, no?

No big deal, both work. A set create a hash table, a list forces a linear search
each time.

Still remove the extraneous [].

[marja, 2012-12-06 16:15:15]

https://codereview.chromium.org/11441035/diff/5001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/11441035/diff/5001/PRESUBMIT.py#newcode617
PRESUBMIT.py:617: changed_linenums = set([line_num for line_num, _ in
f.ChangedContents()])
On 2012/12/06 16:11:26, Marc-Antoine Ruel wrote:
> On 2012/12/06 16:02:53, marja wrote:
> > On 2012/12/06 15:29:27, Marc-Antoine Ruel wrote:
> > > changed_linenums = set(line_num for line_num, _ in f.ChangedContents())
> > > 
> > > but why put it in a set exactly? A list would be fine and faster.
> > 
> > I made the changed_linenums a set, since the check is "are the 2 lines
between
> > which the error is included in the changed lines", so set should be better
for
> > that than a list, no?
> 
> No big deal, both work. A set create a hash table, a list forces a linear
search
> each time.
> 
> Still remove the extraneous [].

Done.

[M-A Ruel, 2012-12-06 16:16:44]

lgtm

[commit-bot: I haz the power, 2012-12-06 16:18:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/11441035/9001

[commit-bot: I haz the power, 2012-12-06 16:18:27]

Failed to apply patch for PRESUBMIT_test.py:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file PRESUBMIT_test.py
  Hunk #1 FAILED at 14.
  Hunk #2 succeeded at 120 (offset 21 lines).
  Hunk #3 succeeded at 129 (offset 21 lines).
  Hunk #4 succeeded at 138 (offset 21 lines).
  Hunk #5 succeeded at 147 (offset 21 lines).
  Hunk #6 succeeded at 192 (offset 21 lines).
  Hunk #7 succeeded at 206 (offset 21 lines).
  Hunk #8 succeeded at 216 (offset 21 lines).
  1 out of 8 hunks FAILED -- saving rejects to file PRESUBMIT_test.py.rej

Patch:       PRESUBMIT_test.py
Index: PRESUBMIT_test.py
diff --git a/PRESUBMIT_test.py b/PRESUBMIT_test.py
index
183ec6e54c839f71ad98827228ce195faa71819e..3c0a7db9173dd811845e4f9786983b273ac33adc
100755
--- a/PRESUBMIT_test.py
+++ b/PRESUBMIT_test.py
@@ -14,6 +14,18 @@ class MockInputApi(object):
   def __init__(self):
     self.re = re
     self.os_path = os.path
+    self.affected_files = []
+
+  def AffectedFiles(self):
+    return self.affected_files
+
+
+class MockOutputApi(object):
+  def __init__(self):
+    self.warnings = []
+
+  def PresubmitPromptWarning(self, legend, warnings):
+    self.warnings = warnings
 
 
 class MockFile(object):
@@ -99,7 +111,7 @@ class IncludeOrderTest(unittest.TestCase):
                 '#include "a/header.h"']
     mock_file = MockFile('some/path/foo.cc', contents)
     warnings = PRESUBMIT._CheckIncludeOrderInFile(
-        mock_input_api, mock_file, True, range(1, len(contents) + 1))
+        mock_input_api, mock_file, range(1, len(contents) + 1))
     self.assertEqual(0, len(warnings))
 
   def testSpecialFirstInclude2(self):
@@ -108,7 +120,7 @@ class IncludeOrderTest(unittest.TestCase):
                 '#include "a/header.h"']
     mock_file = MockFile('some/path/foo.cc', contents)
     warnings = PRESUBMIT._CheckIncludeOrderInFile(
-        mock_input_api, mock_file, True, range(1, len(contents) + 1))
+        mock_input_api, mock_file, range(1, len(contents) + 1))
     self.assertEqual(0, len(warnings))
 
   def testSpecialFirstInclude3(self):
@@ -117,7 +129,7 @@ class IncludeOrderTest(unittest.TestCase):
                 '#include "a/header.h"']
     mock_file = MockFile('some/path/foo_platform.cc', contents)
     warnings = PRESUBMIT._CheckIncludeOrderInFile(
-        mock_input_api, mock_file, True, range(1, len(contents) + 1))
+        mock_input_api, mock_file, range(1, len(contents) + 1))
     self.assertEqual(0, len(warnings))
 
   def testSpecialFirstInclude4(self):
@@ -126,10 +138,19 @@ class IncludeOrderTest(unittest.TestCase):
                 '#include "a/header.h"']
     mock_file = MockFile('some/path/foo_platform.cc', contents)
     warnings = PRESUBMIT._CheckIncludeOrderInFile(
-        mock_input_api, mock_file, True, range(1, len(contents) + 1))
+        mock_input_api, mock_file, range(1, len(contents) + 1))
     self.assertEqual(1, len(warnings))
     self.assertTrue('2' in warnings[0])
 
+  def testSpecialFirstInclude5(self):
+    mock_input_api = MockInputApi()
+    contents = ['#include "some/other/path/foo.h"',
+                '#include "a/header.h"']
+    mock_file = MockFile('some/path/foo-suffix.h', contents)
+    warnings = PRESUBMIT._CheckIncludeOrderInFile(
+        mock_input_api, mock_file, range(1, len(contents) + 1))
+    self.assertEqual(0, len(warnings))
+
   def testOrderAlreadyWrong(self):
     scope = [(1, '#include "b.h"'),
              (2, '#include "a.h"'),
@@ -162,6 +183,10 @@ class IncludeOrderTest(unittest.TestCase):
   def testIfElifElseEndif(self):
     mock_input_api = MockInputApi()
     contents = ['#include "e.h"',
+                '#define foo',
+                '#include "f.h"',
+                '#undef foo',
+                '#include "e.h"',
                 '#if foo',
                 '#include "d.h"',
                 '#elif bar',
@@ -172,7 +197,7 @@ class IncludeOrderTest(unittest.TestCase):
                 '#include "a.h"']
     mock_file = MockFile('', contents)
     warnings = PRESUBMIT._CheckIncludeOrderInFile(
-        mock_input_api, mock_file, True, range(1, len(contents) + 1))
+        mock_input_api, mock_file, range(1, len(contents) + 1))
     self.assertEqual(0, len(warnings))
 
   def testSysIncludes(self):
@@ -182,9 +207,21 @@ class IncludeOrderTest(unittest.TestCase):
                 '#include <sys/a.h>']
     mock_file = MockFile('', contents)
     warnings = PRESUBMIT._CheckIncludeOrderInFile(
-        mock_input_api, mock_file, True, range(1, len(contents) + 1))
+        mock_input_api, mock_file, range(1, len(contents) + 1))
     self.assertEqual(0, len(warnings))
 
+  def testCheckOnlyCFiles(self):
+    mock_input_api = MockInputApi()
+    mock_output_api = MockOutputApi()
+    contents = ['#include <b.h>',
+                '#include <a.h>']
+    mock_file_cc = MockFile('something.cc', contents)
+    mock_file_h = MockFile('something.h', contents)
+    mock_file_other = MockFile('something.py', contents)
+    mock_input_api.affected_files = [mock_file_cc, mock_file_h,
mock_file_other]
+    warnings = PRESUBMIT._CheckIncludeOrder(mock_input_api, mock_output_api)
+    self.assertEqual(2, len(mock_output_api.warnings))
+
 
 class VersionControlerConflictsTest(unittest.TestCase):
   def testTypicalConflict(self):

[marja, 2012-12-06 16:35:37]

(rebased; somebody else was already mocking the output api...)

[commit-bot: I haz the power, 2012-12-06 16:36:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/11441035/14001

[commit-bot: I haz the power, 2012-12-06 16:38:45]

Change committed as 171511