Bug 46099 – check-webkit-style and the coding style guidelines page are inconsistent

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Website

[Balazs Kelemen]

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.

[Balazs Kelemen]

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.

[Balazs Kelemen]

Created attachment 68098 [details]
proposed patch

[Csaba Osztrogonác]

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.

[Balazs Kelemen]

What a terminology guru you are! :)
I agree with the changes you mentioned, those will be adopted in the next patch.

[Balazs Kelemen]

Created attachment 68213 [details]
proposed patch v2

[Csaba Osztrogonác]

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

[David Levin]

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

[Darin Adler]

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.

[Balazs Kelemen]

Landed in http://trac.webkit.org/changeset/68030.
Closing bug