[webkit-reviews] review denied: [Bug 27544] cpplint generates false positives for primary includes : [Attachment 33264] Fix cpplint generating false positives for primary includes

[bugzilla-daemon at webkit.org]

Adam Treat <treat at kde.org> has denied Jakob Petsovits <jpetso at gmx.at>'s request
for review:
Bug 27544: cpplint generates false positives for primary includes
https://bugs.webkit.org/show_bug.cgi?id=27544

Attachment 33264: Fix cpplint generating false positives for primary includes
https://bugs.webkit.org/attachment.cgi?id=33264&action=review

------- Additional Comments from Adam Treat <treat at kde.org>
> Fix false positives for instances when cpplint would
> normally classify multiple includes as primary: After
> the first primary include, classify all other ones as
> "other" includes even if they look like primary ones.

Almost right I think.  Rather, what we want to do is make the primary header
check even more strict after we've found the first one IMO.

> +	   self._visited_primary_section = False
> +	   self.header_types = dict();
> +
> +    def visited_primary_section(self):
> +	   return self._visited_primary_section

Why did you add 'self.header_types = dic();'

>      if target_base.startswith(include_base):
> -	   return _PRIMARY_HEADER
> +	   # If we already had a primary header before, chances are that this
one
> +	   # is a false positive (e.g. "ScrollbarThemeWince.h" and
"Scrollbar.h"
> +	   # might both be classified as primary header, but only the first one

> +	   # is actually it).
> +	   if not include_state.visited_primary_section():
> +	       return _PRIMARY_HEADER

First, you could have combined the two 'if's'.

Second, I'm not sure this is what we want.  Like I said above, I think we want
to make the header check more strict if we've already found the first one. Make
it check for exact match of target_base and include_base if we've already found
one.  And add a testcase for this.
> +	       include_state.header_types[line_number] = header_type

Purpose?

> -		       previous_include = previous_match.group(2)
> -		       previous_header_type = _classify_include(filename,
previous_include, (previous_match.group(1) == '<'))
> +		       previous_header_type =
include_state.header_types[previous_line_number]

Ahh, now I see.  Perhaps is ok, but not strictly necessary for this patch.

> +					     False, include_state))
> +	   # Tricky example where both includes might be classified as primary.

> +	   self.assert_language_rules_check('ScrollbarThemeWince.cpp',
> +					    '#include "config.h"\n'
> +					    '#include
"ScrollbarThemeWince.h"\n'
> +					    '\n'
> +					    '#include "Scrollbar.h"\n',
> +					    '')

Otherwise looks good I think. r- for now.