Bug 39514 - check-webkit-style shouldn't complain about not including a primary header file if none exists.
Summary: check-webkit-style shouldn't complain about not including a primary header fi...
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
: 39787 (view as bug list)
Depends on:
Reported: 2010-05-21 14:14 PDT by David Levin
Modified: 2011-04-15 08:40 PDT (History)
8 users (show)

See Also:

Patch (5.02 KB, patch)
2010-06-04 18:24 PDT, Anders Bakken
no flags Details | Formatted Diff | Diff
Patch (4.96 KB, patch)
2010-06-05 10:36 PDT, Anders Bakken
hamaji: review-
Details | Formatted Diff | Diff
Simple fix (9.11 KB, patch)
2011-04-14 17:14 PDT, Dmitry Lomov
levin: review-
Details | Formatted Diff | Diff
Simple fix + CR addressed (9.22 KB, patch)
2011-04-14 17:36 PDT, Dmitry Lomov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Levin 2010-05-21 14:14:04 PDT
For example, see this comment: https://bugs.webkit.org/show_bug.cgi?id=39327#c4
Comment 1 David Levin 2010-05-27 13:43:58 PDT
*** Bug 39787 has been marked as a duplicate of this bug. ***
Comment 2 Anders Bakken 2010-06-04 18:24:18 PDT
Created attachment 57933 [details]
Comment 3 Anders Bakken 2010-06-05 10:36:45 PDT
Created attachment 57968 [details]
Comment 4 David Levin 2010-06-09 09:25:31 PDT
It would be good if the patch had a test and I'm not certain of some of the changes done.

I've added two folks who know this code much better than I do (and I'm hoping one of them will look this over).
Comment 5 Shinichiro Hamaji 2010-06-11 08:25:03 PDT
Comment on attachment 57968 [details]

Yeah, I think we want some test cases for this change.

 +      def check_next_include_order(self, header_type, file_is_header, include_state):
I think we can just use self as include_state?

 +      for line_number in xrange(clean_lines.num_lines()):
It's unfortunate we need to iterate all lines twice. Cannot we implement this by one-pass approach? Maybe it should be like

1. If primary header comes before config.h, emit a warning when we find config.h
2. If primary header isn't found between config.h and "other headers", set has_primary_header=False
3. If primary header is found after we see at least one "other headers", emit a warning.
Comment 6 Dmitry Lomov 2011-04-14 17:14:35 PDT
Created attachment 89691 [details]
Simple fix

This fix checks whether primary header exists, and only requires the #include if it does.
Comment 7 WebKit Review Bot 2011-04-14 17:17:35 PDT
Attachment 89691 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1

Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py:2615:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/style/checkers/cpp.py:2600:  expected 2 blank lines, found 1  [pep8/E302] [5]
Tools/Scripts/webkitpy/style/checkers/cpp.py:2601:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/style/checkers/cpp.py:2605:  whitespace before ':'  [pep8/E203] [5]
Total errors found: 4 in 3 files

If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 David Levin 2011-04-14 17:25:43 PDT
Comment on attachment 89691 [details]
Simple fix

View in context: https://bugs.webkit.org/attachment.cgi?id=89691&action=review

What the stylebot said plus a few other things to consider.

> Tools/Scripts/webkitpy/style/checkers/cpp.py:315
> +    def check_next_include_order(self, header_type, file_is_header, has_primary_header):

has_primary_header seems ambiguous to me.

What about primary_header_exists?

>> Tools/Scripts/webkitpy/style/checkers/cpp.py:2600
>> +def _has_primary_header(filename):
> expected 2 blank lines, found 1  [pep8/E302] [5]

Consider: _does_primary_header_exist()

> Tools/Scripts/webkitpy/style/checkers/cpp.py:2602
> +    if the file is not source file or primary header does not exist

Add .

> Tools/Scripts/webkitpy/style/checkers/cpp.py:2608
> +    if os.path.isfile(primary_header):

You could just 
  return os.path.isfile(primary_header)

> Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py:2592
> +        # File with non-existing primary header should not produce errors

Add . after comments (and other places).
Comment 9 Dmitry Lomov 2011-04-14 17:36:14 PDT
Created attachment 89701 [details]
Simple fix + CR addressed

CR addressed + style violations (sorry - I keep forgetting running the cop - I will).
Comment 10 WebKit Commit Bot 2011-04-15 08:36:52 PDT
The commit-queue encountered the following flaky tests while processing attachment 89701 [details]:

http/tests/xmlhttprequest/basic-auth-default.html bug 58666 (author: ap@webkit.org)
The commit-queue is continuing to process your patch.
Comment 11 WebKit Commit Bot 2011-04-15 08:40:04 PDT
Comment on attachment 89701 [details]
Simple fix + CR addressed

Clearing flags on attachment: 89701

Committed r83977: <http://trac.webkit.org/changeset/83977>
Comment 12 WebKit Commit Bot 2011-04-15 08:40:09 PDT
All reviewed patches have been landed.  Closing bug.