Summary: | check-webkit-style and the coding style guidelines page are inconsistent | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Balazs Kelemen <kbalazs> | ||||||
Component: | WebKit Website | Assignee: | 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
Balazs Kelemen
2010-09-20 09:21:41 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. Created attachment 68098 [details]
proposed patch
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. What a terminology guru you are! :) I agree with the changes you mentioned, those will be adopted in the next patch. Created attachment 68213 [details]
proposed patch v2
(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. (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 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.
Landed in http://trac.webkit.org/changeset/68030. Closing bug |