Bug 51523 – check-webkit-style check for meaningless variable names in function declarations.

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

[David Levin]

See summary.

[David Levin]

Created attachment 77312 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 77312 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77312&action=review

Have you run this across the entire source base to check for false positives?  I worry there may be a bunch.

> Tools/Scripts/webkitpy/style/checkers/cpp.py:215
> +def _create_acronym(text):
> +    # Removes all lower case letters except those starting words.
> +    text = sub(r'(?<!\b)[a-z]', '', text)
> +    return text.upper()

Woh.  So this is used for finding "ec" used with "ExceptionCode", right?  Probably best to add a comment about how it's used?

> Tools/Scripts/webkitpy/style/checkers/cpp.py:1437
> +        parameter_type = parameter[0]
> +        parameter_name = parameter[1]
> +        parameter_line_number = parameter[2]

Eeek.  This is a class. :)

> Tools/Scripts/webkitpy/style/checkers/cpp.py:1440
> +        if not len(parameter_name):
> +            continue

if not parameter_name works too.

> Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py:-162
> -                             '-multi_line_filter',

?  bogus filter?

[David Levin]

(In reply to comment #2)
> (From update of attachment 77312 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77312&action=review
> 
> Have you run this across the entire source base to check for false positives?  I worry there may be a bunch.

I agree that it is something really, really important for this style check. Previously, I've run this across JavaScriptCore, WebCore and WebKit like this:
   check-webkit-style -f -,+readability/parameter_name WebCore/ 
to check for false positives (and crashes).

As far as I can tell, I have eliminated all false positives except for: JavaScriptCore/jit/JITStubs.cpp

That file has an unusual syntax, and now that I think about it, I think I will just exempt it from this check to get rid of that false positive.

> 
> > Tools/Scripts/webkitpy/style/checkers/cpp.py:215
> > +def _create_acronym(text):
> > +    # Removes all lower case letters except those starting words.
> > +    text = sub(r'(?<!\b)[a-z]', '', text)
> > +    return text.upper()
> 
> Woh.  So this is used for finding "ec" used with "ExceptionCode", right?  Probably best to add a comment about how it's used?

Yep :) Will do.

> 

I'll fix the rest.

[David Levin]

Comment on attachment 77312 [details]
Patch

I need to do some significant revisions to address these comments and the items in the dependent bug, so I'm clearing the r+ for now.

[David Levin]

Created attachment 77484 [details]
Patch

[David Levin]

Comment on attachment 77484 [details]
Patch

> > Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py:-162
> > -                             '-multi_line_filter',
> 
> ?  bogus filter?
Yep. Since I was adding a filter in there, I noticed this bogus one. It is in code that I recently changed.

[Eric Seidel (no email)]

Comment on attachment 77484 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77484&action=review

Will have to look again when I'm less tired.

> Tools/Scripts/webkitpy/style/checkers/cpp.py:354
> +        # None is a placeholder as this value is computer on demand.

computed. :)  Also, you could just have this be _cached_foo and tehn you don't need to expain that its' on-demand, or?

[David Levin]

Created attachment 77485 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 77485 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77485&action=review

LGTM.  Feel free to land as is.

> Tools/Scripts/webkitpy/style/checkers/cpp.py:210
> +    text = sub(r'(?<=[A-Za-z0-9])([A-Z])(?=[a-z])', r'_\1', text)
> +
> +    # Next add underscores before capitals at the end of words if it was
> +    # preceeded by lower case letter or number.
> +    # (This puts an underscore before A in isA but not A in CBA).
> +    text = sub(r'(?<=[a-z0-9])([A-Z])(?=\b)', r'_\1', text)
> +
> +    # Next add underscores when you have a captial letter which is followed by a capital letter
> +    # but is not proceeded by one. (This puts an underscore before A in 'WordADay').
> +    text = sub(r'(?<=[a-z0-9])([A-Z][A-Z_])', r'_\1', text)

If you were worried about speed (which it sounds like you are a little based on having built a cache method), you could pre-compile these regexps and save them on the class.  (aka, just define them with _foo = re.compile() on the class and then access them with self._foo)

> Tools/Scripts/webkitpy/style/checkers/cpp.py:214
> +    # Finally lower case the string and remove an initial underscore if present.
> +    text = text.lower()
> +    return text

This could just be one line. :)  And the comment looks slightly out of date, sinc eyou don't remove the underscore here.  Looks like your unit tests don't pass leading underscores, should they?

> Tools/Scripts/webkitpy/style/checkers/cpp.py:360
> +    def lower_with_underscores_name(self):
> +        """Returns the parameter name in the lower with underscores format."""
> +        if self._cached_lower_with_underscores_name is None:
> +            self._cached_lower_with_underscores_name = _convert_to_lower_with_underscores(self.name)
> +        return self._cached_lower_with_underscores_name

Sorry, I should have mentioned earlier that you can just use the @memoized decorator if you'd like.  I'm not sure it matters.  It's in common.meomized iirc.

> Tools/Scripts/webkitpy/style/checkers/cpp.py:1503
> +        error(parameter.row, 'readability/parameter_name', 5,

I need to come up with a good replacement for this error function someday.

> Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py:122
> +    def test_convert_to_lower_with_underscores(self):
> +        self.assertEquals(cpp_style._convert_to_lower_with_underscores('ABC'), 'abc')
> +        self.assertEquals(cpp_style._convert_to_lower_with_underscores('aB'), 'a_b')
> +        self.assertEquals(cpp_style._convert_to_lower_with_underscores('isAName'), 'is_a_name')
> +        self.assertEquals(cpp_style._convert_to_lower_with_underscores('AnotherTest'), 'another_test')
> +        self.assertEquals(cpp_style._convert_to_lower_with_underscores('PassRefPtr<MyClass>'), 'pass_ref_ptr<my_class>')
> +
> +    def test_create_acronym(self):
> +        self.assertEquals(cpp_style._create_acronym('ABC'), 'ABC')
> +        self.assertEquals(cpp_style._create_acronym('IsAName'), 'IAN')
> +        self.assertEquals(cpp_style._create_acronym('PassRefPtr<MyClass>'), 'PRP<MC>')

I'm *so* glad you added these.  So helpful.

[David Levin]

(In reply to comment #9)
> (From update of attachment 77485 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77485&action=review
> 
> LGTM.  Feel free to land as is.

Hurray! So happy to get in this final step.

Landed as http://trac.webkit.org/changeset/74688

> 
> > Tools/Scripts/webkitpy/style/checkers/cpp.py:210
> > +    text = sub(r'(?<=[A-Za-z0-9])([A-Z])(?=[a-z])', r'_\1', text)
> > +
> > +    # Next add underscores before capitals at the end of words if it was
> > +    # preceeded by lower case letter or number.
> > +    # (This puts an underscore before A in isA but not A in CBA).
> > +    text = sub(r'(?<=[a-z0-9])([A-Z])(?=\b)', r'_\1', text)
> > +
> > +    # Next add underscores when you have a captial letter which is followed by a capital letter
> > +    # but is not proceeded by one. (This puts an underscore before A in 'WordADay').
> > +    text = sub(r'(?<=[a-z0-9])([A-Z][A-Z_])', r'_\1', text)
> 
> If you were worried about speed (which it sounds like you are a little based on having built a cache method), you could pre-compile these regexps and save them on the class.  (aka, just define them with _foo = re.compile() on the class and then access them with self._foo)

fwiw, the style code uses a pattern (which I think you dislike) of calling a local sub (etc.) function which caches the regex in a hashtable based on the string.


> 
> > Tools/Scripts/webkitpy/style/checkers/cpp.py:214
> > +    # Finally lower case the string and remove an initial underscore if present.
> > +    text = text.lower()
> > +    return text
> 
> This could just be one line. :)
Fixed.

>  And the comment looks slightly out of date, sinc eyou don't remove the underscore here. 

Thanks! Removed. (At one point my regex added an underscore at the front of the string and this was a clean-up step, but I made the regex better to avoid this.)

>  Looks like your unit tests don't pass leading underscores, should they?

Added one just for fun.

> > Tools/Scripts/webkitpy/style/checkers/cpp.py:360
> > +    def lower_with_underscores_name(self):
> > +        """Returns the parameter name in the lower with underscores format."""
> > +        if self._cached_lower_with_underscores_name is None:
> > +            self._cached_lower_with_underscores_name = _convert_to_lower_with_underscores(self.name)
> > +        return self._cached_lower_with_underscores_name
> 
> Sorry, I should have mentioned earlier that you can just use the @memoized decorator if you'd like.  I'm not sure it matters.  It's in common.meomized iirc.

Changed to use meomized. That is indeed simpler. I should consider using this for the FunctionDetection.parameter_list as well.