Bug 39514

*** Bug 39787 has been marked as a duplicate of this bug. ***

Created attachment 57933 [details] Patch

Created attachment 57968 [details] Patch

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 on attachment 57968 [details] Patch Yeah, I think we want some test cases for this change. WebKitTools/Scripts/webkitpy/style/checkers/cpp.py:224 + def check_next_include_order(self, header_type, file_is_header, include_state): I think we can just use self as include_state? WebKitTools/Scripts/webkitpy/style/checkers/cpp.py:2875 + 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.

Created attachment 89691 [details] Simple fix This fix checks whether primary header exists, and only requires the #include if it does.

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 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).

Created attachment 89701 [details] Simple fix + CR addressed CR addressed + style violations (sorry - I keep forgetting running the cop - I will).

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 on attachment 89701 [details] Simple fix + CR addressed Clearing flags on attachment: 89701 Committed r83977: <http://trac.webkit.org/changeset/83977>

All reviewed patches have been landed. Closing bug.