[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 #5 from Adam Treat <treat at kde.org>  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!

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