Bug 27544 – cpplint generates false positives for primary includes

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jakob Petsovits]

The test for primary includes is intentionally broad in order to catch platform-specific variations (e.g. "BlahQt.h", "BlahWince.h", etc.). However, that might lead to a situation where an include file is classified as primary include even though it's not. The patch below avoids this situation for code where cpplint would detect multiple primary includes - obviously, only one can be the real thing, which is (hopefully) the first one.

By introducing a bit more state into the _IncludeState class, subsequent "primary" includes after the actual (first) one can be treated as regular "other" includes, fixing the false positive. See the test case for a less abstract example of this situation.

Brought to attention by Yong Li.

[Jakob Petsovits]

Created attachment 33264 [details]
Fix cpplint generating false positives for primary includes

[Adam Treat]

Comment on attachment 33264 [details]
Fix cpplint generating false positives for primary includes

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

[Jakob Petsovits]

Created attachment 33272 [details]
Fix cpplint generating false positives for primary includes (try 2)

Combined the two ifs into one condition in this new patch. This also makes it apparent that there is no need to make the check stricter than startswith() after encountering the first occurrence, because it won't be triggered anyways.

> (regarding header_types dict)
> Ahh, now I see.  Perhaps is ok, but not strictly necessary for this patch.

It was actually necessary, because the second _classify_include() apparently caused some state change which in turn caused some test case to fail. So this change makes the first _classify_include() to be the only one that's being called, which ensures proper state tracking.

[Jakob Petsovits]

Created attachment 33279 [details]
Fix cpplint generating false positives for primary includes (try 3)

Ok, new try: This one still regards subsequent includes as primary if they strictly match the implementation's filename. Plus a new testcase to verify that.

[Jakob Petsovits]

Created attachment 33280 [details]
Fix cpplint generating false positives for primary includes (try 4)

One more try, with (hopefully) clearer comments, incorporating suggestions by Adam Treat on IRC.

[Adam Treat]

Comment on attachment 33280 [details]
Fix cpplint generating false positives for primary includes (try 4)

Looks good.

[Adam Treat]

Landed with r46230.