[webkit-reviews] review granted: [Bug 27333] cpplint should check for equality comparisons to 0/true/false : [Attachment 32874] Updated patched.

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has granted David Levin
<levin at chromium.org>'s request for review:
Bug 27333: cpplint should check for equality comparisons to 0/true/false
https://bugs.webkit.org/show_bug.cgi?id=27333

Attachment 32874: Updated patched.
https://bugs.webkit.org/attachment.cgi?id=32874&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
> +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.