[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


David Levin <levin at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #33175|review?                     |review-
               Flag|                            |




--- Comment #4 from David Levin <levin at chromium.org>  2009-07-21 09:34:20 PDT ---
(From update of attachment 33175)
This looks really good.  Just a few things to address.

> diff --git a/WebKitTools/Scripts/modules/cpplint.py b/WebKitTools/Scripts/modules/cpplint.py
> +        if header_type == _CONFIG_HEADER and file_is_header:
> +            return 'Header file should not contain WebCore config.h.'
> +        if header_type == _PRIMARY_HEADER and file_is_header:
> +            return 'Header file should not contain itself.'
> +
> +        error_message = ''
> +        if self._section != self._OTHER_SECTION:
> +            before_error_message = ('Found %s before %s' %
> +                                    (self._TYPE_NAMES[header_type],
> +                                     self._SECTION_NAMES[self._section + 1]))
> +        after_error_message = ('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:
> +                error_message = after_error_message
> +            self._section = self._CONFIG_SECTION
> +        elif header_type == _PRIMARY_HEADER:
> +            if self._section >= self._PRIMARY_SECTION:
> +                error_message = after_error_message
> +            elif self._section < self._CONFIG_SECTION:
> +                error_message = before_error_message

I think this is primarily here to note that the config.h header include may be
missing.  

Maybe it is better just have the message say something about the missing
include.
"It appears that config.h wasn't included (or it wasn't included first)."


> +            if not file_is_header and self._section < self._PRIMARY_SECTION:

This seems to be ensuring that we got to the primary header.  It is rare but
sometimes there is no primary header.
I think the only thing to check here is that the missing config.h file.  The
check becomes:
   if not file_is_header and self._section < self._CONFIG_SECTION:

Have the same message as before about missing config.h.  This gets rid of
before_error_message by the way.


> +            # Check to make sure all headers besides config.h and the primary header are
> +            # alphabetically sorted.
> +            if not error_message and header_type == _OTHER_HEADER:
> +                 previous_line_number = line_number - 1;
> +                 previous_line = clean_lines.lines[previous_line_number]
> +                 previous_match = _RE_PATTERN_INCLUDE.search(previous_line)
> +                 while not previous_match and previous_line_number > 0:
> +                    previous_line_number -= 1;
> +                    previous_line = clean_lines.lines[previous_line_number]
> +                    previous_match = _RE_PATTERN_INCLUDE.search(previous_line)

I would try to match #ifdef/#else here and break out if you get that without a
previous match (just to avoid flagging a sorting issue when there is an
ifdef'ed group after the regular includes.



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

> +    def test_check_next_include_order__no_config(self):
> +        self.assertEqual('Header file should not contain WebCore config.h.',
>                           self.include_state.check_next_include_order(
> -                             cpplint._CPP_SYS_HEADER))
> +                            cpplint._CONFIG_HEADER, True))
You seem to be doing a two space indenting here.  Anyway, I just wouldn't wrap
the arguments.


It would be nice to add a test for this case as well:

   #include "test.h"

   #ifdef MY_DEFINE
   #include "a.h"
   #endif
Just to verify that a.h ins't flagged as a sorting error.



There is a test that says this:
3097        def test_include(self):
3098            # FIXME: Implement this.
3099            pass

Should this test be deleted (or just add a comment there saying where it is
tested) or should the header tests move there? etc.

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