Bug 27462 - Add cpplint check for proper include order
Summary: Add cpplint check for proper include order
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-20 15:00 PDT by Adam Treat
Modified: 2009-07-21 11:17 PDT (History)
0 users

See Also:


Attachments
Checks include order and tests (28.35 KB, patch)
2009-07-20 15:01 PDT, Adam Treat
no flags Details | Formatted Diff | Diff
New version incorporating comments (31.42 KB, patch)
2009-07-21 07:00 PDT, Adam Treat
levin: review-
Details | Formatted Diff | Diff
More changes reflecting comments (34.53 KB, patch)
2009-07-21 10:50 PDT, Adam Treat
no flags Details | Formatted Diff | Diff
New version that special cases 'FooCustom.cpp' (35.07 KB, patch)
2009-07-21 11:09 PDT, Adam Treat
levin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Treat 2009-07-20 15:00:05 PDT
cpplint should check to make sure that header files and implementation files follow the proper include order and style mandated here:

http://webkit.org/coding/coding-style.html

The only thing not checked here is whether includes surrounded by ifdefs are put at the end.
Comment 1 Adam Treat 2009-07-20 15:01:35 PDT
Created attachment 33109 [details]
Checks include order and tests
Comment 2 David Levin 2009-07-20 20:15:05 PDT
Comment on attachment 33109 [details]
Checks include order and tests

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@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).
Comment 3 Adam Treat 2009-07-21 07:00:06 PDT
Created attachment 33175 [details]
New version incorporating comments

I've greatly simplified _classify_include following discussions on IRC and incorporated your other comments into this new version.
Comment 4 David Levin 2009-07-21 09:34:20 PDT
Comment on attachment 33175 [details]
New version incorporating comments

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.
Comment 5 Adam Treat 2009-07-21 10:23:28 PDT
(In reply to comment #4)
> (From update of attachment 33175 [details])
> This looks really good.  Just a few things to address.
...
> 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)."

It is there to note the correct order, not just to see if the config.h is missing.  It does say something if there is a missing include whether it is a config header or the primary header.
 
The error messages look like this (from the test cases):

    "Found header this file implements before WebCore config.h."

and

    "Found other header before WebCore config.h."

and

    "Found other header before a header this file implements."

I think these are quite clear error messages and the test cases encompass the range of possible errors pretty well.  I don't see any need to simplify further.

> 
> > +            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:

I think this is incorrect.  As you say, it is rare that an implementation file does not have a primary header.  I see no reason to ignore the check for the vast majority of cases to stop a false positive on the rare corner case.  That is why the error message has a confidence of 4 and not 5.  We could bump this down to 3 if you like, but I think the check should stay.
 
> 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.

Ok, will do.
 
> > +    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.

Ok, will do.

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

Ok, will do.
 
> 
> 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.

I'll remove it.

Thanks...

Patch to follow!
Comment 6 Adam Treat 2009-07-21 10:50:20 PDT
Created attachment 33187 [details]
More changes reflecting comments

New version reflecting the state of the review conversation.
Comment 7 Adam Treat 2009-07-21 11:09:02 PDT
Created attachment 33191 [details]
New version that special cases 'FooCustom.cpp'

New version that special cases 'FooCustom.cpp' as discussed on IRC.
Comment 8 David Levin 2009-07-21 11:11:54 PDT
Comment on attachment 33191 [details]
New version that special cases 'FooCustom.cpp'

Would be nice to make s small adjustment on landing.

> +                if filename.endswith('.h'):
> +                    error(filename, line_number, 'build/include_order', 4,
> +                          '%s Should be: alphabetically sorted.' %
> +                          (error_message))
No need for parens on error_message (since it is a single element).

Btw, wierd python-ism: the parens do nothing here :) 
to make it a tuple with one element you'd have to do this: (error_message,)
 


> +                else:
> +                    error(filename, line_number, 'build/include_order', 4,
> +                          '%s Should be: config.h, primary header, blank line, and then alphabetically sorted.' %
> +                          (error_message))

Same comment about (error_message)
Comment 9 Adam Treat 2009-07-21 11:17:50 PDT
Landed with last changes mentioned with r46181.