[Webkit-unassigned] [Bug 27462] Add cpplint check for proper include order

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=27462





--- Comment #2 from David Levin <levin at chromium.org>  2009-07-20 20:15:05 PDT ---
(From update of attachment 33109)
Here some initial comments to consider.  The checking of header states really
took me some time to get through (and I need to look at it a little more). 
Since I'm not done, I won't give this an r-.


> diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
> +2009-07-20  Adam Treat  <adam.treat at torchmobile.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Add a new check to cpplint to flag cases where the include section of a file
> +        does not match the mandated include order and style of the Webkit coding style
> +        guidelines.
> +
> +        Add associated tests.
> +        https://bugs.webkit.org/show_bug.cgi?id=27462

I think the style du jour for WebKit changelog's is to have it like this:

bug title
bug link

description

> diff --git a/WebKitTools/Scripts/modules/cpplint.py b/WebKitTools/Scripts/modules/cpplint.py

> @@ -273,31 +272,28 @@ class _IncludeState(dict):
>      # self._section will move monotonically through this set. If it ever
>      # needs to move backwards, check_next_include_order will raise an error.

Note the comment for the "class _IncludeState(dict):"

 "Calls in an illegal order will raise an _IncludeError with an appropriate
error message."

is incorrect as it was before you started your change (but time to fix it).



> +        errorMessage = ''

You switchedToWebkitCStyleNames instead of python_style_names (note:
beforeErrorMessage and afterErrorMessage as well).

> +        if (self._section != self._OTHER_H_SECTION):
> +            beforeErrorMessage = ('Found %s before %s' %
> +                                    (self._TYPE_NAMES[header_type],
> +                                     self._SECTION_NAMES[self._section + 1]));
1. The indenting appears to be off.
2. No need for ; in python.

> +        afterErrorMessage = ('Found %s after %s' %
> +                                (self._TYPE_NAMES[header_type],
> +                                 self._SECTION_NAMES[self._section]));
> +


> +        if header_type == _CONFIG_HEADER:
> +            if self._section > self._CONFIG_SECTION:
Perhaps:
            if self._section >= self._CONFIG_SECTION:

since there is only one item in the config section.


>          elif header_type == _LIKELY_MY_HEADER:
> +            if self._section == self._OTHER_H_SECTION:

Consider:
            if self._section > self._MY_H_SECTION:


>              assert header_type == _OTHER_HEADER
> +            if not file_is_header and self._section != self._MY_H_SECTION and self._section != self._OTHER_H_SECTION:
> +                errorMessage = beforeErrorMessage

I think this is to catch missing config.h files.  Maybe the error should just
say that "This cpp files appears to not include config.h or config,h is not
included first."


> +                 if (not is_blank_line(next_line)):

I've noticed that you did a lot of if statements with surrounding ().  There
isn't a need for this (unless you want to continue the condition on to a new
line).

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.