RESOLVED FIXED Bug 32991
[Qt] check-webkit-style generates false report on "#include *.moc" 
https://bugs.webkit.org/show_bug.cgi?id=32991
Summary [Qt] check-webkit-style generates false report on "#include *.moc"
Chang Shu
Reported 2009-12-28 12:33:27 PST
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.
Attachments
fix patch (1.37 KB, patch)
2009-12-28 12:43 PST, Chang Shu 		eric: review-
DetailsFormatted DiffDiff
alternative fix (1.88 KB, patch)
2009-12-29 10:15 PST, Chang Shu 		abarth: review-
DetailsFormatted DiffDiff
added unit tests (3.04 KB, patch)
2009-12-29 13:19 PST, Chang Shu 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Chang Shu
Comment 1 2009-12-28 12:43:32 PST
Created attachment 45568 [details] fix patch Include files ending with ".moc" were accidentally categorized as PRIMARY_HEADER.
WebKit Review Bot
Comment 2 2009-12-28 12:47:10 PST
style-queue ran check-webkit-style on attachment 45568 [details] without any errors.
Eric Seidel (no email)
Comment 3 2009-12-29 00:06:09 PST
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
Comment 4 2009-12-29 06:53:51 PST
(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
Comment 5 2009-12-29 10:15:31 PST
Created attachment 45612 [details] alternative fix This is an alternative fix. It simply changes the order of processing headers.
WebKit Review Bot
Comment 6 2009-12-29 10:19:19 PST
style-queue ran check-webkit-style on attachment 45612 [details] without any errors.
Adam Barth
Comment 7 2009-12-29 11:47:21 PST
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
Comment 8 2009-12-29 13:19:24 PST
Created attachment 45620 [details] added unit tests 2 new unit tests added for Qt moc headers.
WebKit Review Bot
Comment 9 2009-12-29 13:21:17 PST
style-queue ran check-webkit-style on attachment 45620 [details] without any errors.
Adam Barth
Comment 10 2009-12-29 13:58:35 PST
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 11 2009-12-29 15:06:40 PST
Comment on attachment 45620 [details] added unit tests Clearing flags on attachment: 45620 Committed r52636: <http://trac.webkit.org/changeset/52636>
WebKit Commit Bot
Comment 12 2009-12-29 15:06:48 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.