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.
Created attachment 45568 [details] fix patch Include files ending with ".moc" were accidentally categorized as PRIMARY_HEADER.
style-queue ran check-webkit-style on attachment 45568 [details] without any errors.
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.
(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.
Created attachment 45612 [details] alternative fix This is an alternative fix. It simply changes the order of processing headers.
style-queue ran check-webkit-style on attachment 45612 [details] without any errors.
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.
Created attachment 45620 [details] added unit tests 2 new unit tests added for Qt moc headers.
style-queue ran check-webkit-style on attachment 45620 [details] without any errors.
Comment on attachment 45620 [details] added unit tests LGTM. A cpp_lint expert might want to double check. Thanks for the test.
Comment on attachment 45620 [details] added unit tests Clearing flags on attachment: 45620 Committed r52636: <http://trac.webkit.org/changeset/52636>
All reviewed patches have been landed. Closing bug.