Bug 27333 – cpplint should check for equality comparisons to 0/true/false

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

[David Levin]

According to http://webkit.org/coding/coding-style.html,
"Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons."

[David Levin]

Created attachment 32839 [details]
Proposed fix.

[Shinichiro Hamaji]

Though I'm not a reviewer, it would make sense I comment on this patch as I introduced the linter. This looks great, thanks for this. A few comments:

- Maybe this rule will warn for "font == truetype" or something like this? Actually, it seems that HTMLMarqueeElement.cpp has "attr->name() == truespeedAttr".
- Could you update WebKitStyleTest.test_null_false_zero using the example of the style guide? We can see the list of supported rules by this.

[David Levin]

Created attachment 32849 [details]
Proposed fix.

[David Levin]

Nice catch on that bug Shinichiro.  I've fixed it and moved the test case as suggested.

I did one one other small fix to the unit tests in this bug: I changed a regex in another test because it was failing due to a number in one of my directory names (WebKit-2).

[David Levin]

Created attachment 32872 [details]
Proposed fix (that addresses previous comments and has an updated ChangeLog).

[David Levin]

Created attachment 32874 [details]
Updated patched.

Fixed the (ironic) incorrectly styled variable names in the test cases.

[David Kilzer (:ddkilzer)]

Comment on attachment 32874 [details]
Updated patched.

> +def check_for_comparisons_to_zero(filename, clean_lines, line_number, error):
> +    # Get the line without comments and strings.
> +    line = clean_lines.elided[line_number]
> +
> +    # Include NULL here so that users don't have to convert NULL to 0 first and then get this error.
> +    if search(r'[=!]=\s*(NULL|0|true|false)\W', line) or search(r'\W(NULL|0|true|false)\s*[=!]=', line):
> +        error(filename, line_number, 'readability/comparison_to_zero', 5,
> +	      'Tests for true/false, zero/non-zero, etc. should be done without equality comparisons.')

I think the error message should be more specific:

      'Tests for true/false, zero/non-zero, NULL/not-NULL should be done without equality comparisons.')

> @@ -2907,8 +2904,29 @@ class WebKitStyleTest(CpplintTestBase):
>  
>          # 3. Tests for true/false, null/non-null, and zero/non-zero should
>          #    all be done without equality comparisons.
> -        # FIXME: Implement this.
> -        pass
> +        self.assert_lint(
> +            'if (count == 0)',
> +            'Tests for true/false, zero/non-zero, etc. should be done without equality comparisons.'
> +            '  [readability/comparison_to_zero] [5]')
> +        self.assert_lint(
> +            'if (string == NULL)',
> +            'Tests for true/false, zero/non-zero, etc. should be done without equality comparisons.'
> +            '  [readability/comparison_to_zero] [5]')
> +        self.assert_lint(
> +            'if (0 != aLongerVariableName)',
> +            'Tests for true/false, zero/non-zero, etc. should be done without equality comparisons.'
> +            '  [readability/comparison_to_zero] [5]')
> +        self.assert_lint(
> +            'if (condition == true)',
> +            'Tests for true/false, zero/non-zero, etc. should be done without equality comparisons.'
> +            '  [readability/comparison_to_zero] [5]')
> +        self.assert_lint(
> +            'if (myVariable == /* Why would anyone put a comment here? */ false)',
> +            'Tests for true/false, zero/non-zero, etc. should be done without equality comparisons.'
> +            '  [readability/comparison_to_zero] [5]')
> +        self.assert_lint(
> +            'if (fontType == trueType)',
> +            '')

The error messages here need to be updated to match the changed error message.

Ideally, all permutations of lhs, rhs and ==/!= should be tested here, but I think this is sufficient.

r=me with the error message change.

[David Levin]

Committed as http://trac.webkit.org/changeset/45981