[Webkit-unassigned] [Bug 27544] cpplint generates false positives for primary includes

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=27544


Adam Treat <treat at kde.org> changed:

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




--- Comment #2 from Adam Treat <treat at kde.org>  2009-07-22 09:30:09 PDT ---
(From update of attachment 33264)
> 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.

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