[webkit-reviews] review granted: [Bug 51523] check-webkit-style check for meaningless variable names in function declarations. : [Attachment 77485] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted David Levin <levin at chromium.org>'s
request for review:
Bug 51523: check-webkit-style check for meaningless variable names in function
declarations.
https://bugs.webkit.org/show_bug.cgi?id=51523

Attachment 77485: Patch
https://bugs.webkit.org/attachment.cgi?id=77485&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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<MyCl
ass>'), '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.