Bug 46099

Summary: check-webkit-style and the coding style guidelines page are inconsistent
Product: WebKit Reporter: Balazs Kelemen <kbalazs>
Component: WebKit WebsiteAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, eric, levin, ossy
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
proposed patch
none
proposed patch v2 none

Description Balazs Kelemen 2010-09-20 09:21:41 PDT
The code style guideline page says: 
"Other #include statements should be in sorted order (case sensitive, as done by the
command-line sort tool or the Xcode sort selection command)".
According to that an ordering is ok if sort do not reorders it.

This one is ordered as sort would do:
------------------------
#include "Chrome.h"
#include "ChromeClientQt.h"
#include <IntSize.h>
#include "NotImplemented.h"
#include <Page.h>
------------------------

check-webkit-style does not accept this. It accepts the following:
------------------------
#include "Chrome.h"
#include "ChromeClientQt.h"
#include "NotImplemented.h"

#include <IntSize.h>
#include <Page.h>
------------------------

I agree that we should separate the system style includes from the normal ones but we should
make it clear in the style guidelines.
Comment 1 Balazs Kelemen 2010-09-20 09:46:18 PDT
I have realized that check-webkit-style does not expect grouping.
It accepts the following:
------------------------
#include "Chrome.h"
#include "ChromeClientQt.h"
#include "NotImplemented.h"
#include <IntSize.h>
#include <Page.h>
------------------------

So the rule is that system style includes should not come before
normal style ones.
Comment 2 Balazs Kelemen 2010-09-20 09:49:32 PDT
Created attachment 68098 [details]
proposed patch
Comment 3 Csaba Osztrogonác 2010-09-20 23:46:57 PDT
Comment on attachment 68098 [details]
proposed patch

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

I have never seen "system style" and "normal style" includes in C terminology,
but system headers. See: http://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html#Include-Syntax

Otherwise LGTM, making clear this style issue is necessary.

> WebKitSite/ChangeLog:9
> +        * coding/coding-style.html: Making clear that system style includes
> +        should not come before normal ones.

I prefer like this:
Making clear that includes of system headers must come after includes of other headers.

> WebKitSite/coding/coding-style.html:759
> +<li>System syle includes should not come before normal ones.

I prefer like this:
Includes of system headers must come after includes of other headers.
Comment 4 Balazs Kelemen 2010-09-21 03:58:01 PDT
What a terminology guru you are! :)
I agree with the changes you mentioned, those will be adopted in the next patch.
Comment 5 Balazs Kelemen 2010-09-21 04:01:18 PDT
Created attachment 68213 [details]
proposed patch v2
Comment 6 Csaba Osztrogonác 2010-09-21 04:07:20 PDT
(In reply to comment #5)
> Created an attachment (id=68213) [details]
> proposed patch v2

LGTM.

I cc-ed Darin, he is the best person for reviewing website modification.
Comment 7 David Levin 2010-09-21 16:17:48 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > Created an attachment (id=68213) [details] [details]
> > proposed patch v2
> 
> LGTM.
> 
> I cc-ed Darin, he is the best person for reviewing website modification.

I'd suggest an email to webkit-dev about this change. If people are fine with it, then r+'ing this will be easy.

  I think this practice is already widely done and recommended, just not in the styleguide.
Comment 8 Darin Adler 2010-09-21 17:45:24 PDT
Comment on attachment 68213 [details]
proposed patch v2

I think it’s fine to make this clearer in this fashion. Ordered as “sort” would do was intended to be ordered as sort would sort the entire source lines, not just the filenames, and so that’s why there’s no separate rule about this today.

If it was me, I would simply add some system header to the existing example instead of making this a separate rule, but I’m OK with it like this too.

I don’t think this really requires webkit-dev discussion. This rule has not changed.
Comment 9 Balazs Kelemen 2010-09-22 02:05:29 PDT
Landed in http://trac.webkit.org/changeset/68030.
Closing bug