Bug 32991 – [Qt] check-webkit-style generates false report on "#include *.moc"

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Chang Shu]

check-webkit-style generated the following error report on patch 45564 in bug 32614.
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/wtf/qt/ThreadingQt.cpp:293:  Found header this file implements
after other header. Should be: config.h, primary header, blank line, and then
alphabetically sorted.  [build/include_order] [4]
Total errors found: 1

I think it's a false alarm.
Lines like "#include ThreadingQt.moc" in a Qt cpp file should not follow the general include style.

[Chang Shu]

Created attachment 45568 [details]
fix patch

Include files ending with ".moc" were accidentally categorized as PRIMARY_HEADER.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45568 [details] without any errors.

[Eric Seidel (no email)]

Comment on attachment 45568 [details]
fix patch

This requires a unittest.  I'm not familiar enough with this code to know if this is the right approach.

[Chang Shu]

(In reply to comment #3)
> (From update of attachment 45568 [details])
> This requires a unittest.  I'm not familiar enough with this code to know if
> this is the right approach.

The line I have changed is to check the primary header by comparing the base file name. For example, if the cpp file is test.cpp, test.h will be the primary header. However, for Qt, a line such as "#include test.moc" may show up at the end of the file. Its base name is also "test" but we should exclude this from primary header.

[Chang Shu]

Created attachment 45612 [details]
alternative fix

This is an alternative fix. It simply changes the order of processing headers.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45612 [details] without any errors.

[Adam Barth]

Comment on attachment 45612 [details]
alternative fix

I'm not a cpp_lint expert, but this second patch looks like a good approach.  However, we still need a test.  cpp_lint has extensive unit tests.  It should be straightforward to modify one to test this new behavior.

[Chang Shu]

Created attachment 45620 [details]
added unit tests

2 new unit tests added for Qt moc headers.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45620 [details] without any errors.

[Adam Barth]

Comment on attachment 45620 [details]
added unit tests

LGTM.  A cpp_lint expert might want to double check.  Thanks for the test.

[WebKit Commit Bot]

Comment on attachment 45620 [details]
added unit tests

Clearing flags on attachment: 45620

Committed r52636: <http://trac.webkit.org/changeset/52636>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.