[webkit-reviews] review denied: [Bug 27462] Add cpplint check for proper include order : [Attachment 33175] New version incorporating comments

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Adam Treat <treat at kde.org>'s
request for review:
Bug 27462: Add cpplint check for proper include order
https://bugs.webkit.org/show_bug.cgi?id=27462

Attachment 33175: New version incorporating comments
https://bugs.webkit.org/attachment.cgi?id=33175&action=review

------- Additional Comments from David Levin <levin at chromium.org>
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.