[Webkit-unassigned] [Bug 32051] check-webkit-style should check for camelCase variable names

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=32051





--- Comment #11 from Shinichiro Hamaji <hamaji at chromium.org>  2009-12-03 04:40:29 PST ---
Thanks for the detailed comments! It helped me a lot.

I think I addressed all small issues such as typo, variable names, and
wrong English. Also, I wrote more comments, sorry for poor comments in
the previous patch.

It was very bad that I didn't write docstring which kind of
identifiers the function I added will check, sorry. The function I
added only checks identifiers in their declaration. If we check the
use of identifiers, it would be very difficult to avoid false alarms
because there would be several identifiers which violates our style
guide in libraries we are using. I also wrote this in the docstring.

I think this decision can be objected. If the checker complains about
the use of wrongly named symbols, we will have more chance to fix
them when we are just using them. However, I think the benefit to
reduce the risk of false alarming for third-party's variables wins the
benefit of more warnings. Also, we may need to add some exception
lists for static_cast or something like this. What do you think?

> Why not just avoid flagging TEXT_THAT_LOOKS_LIKE_THIS since this is
> a defined value

At first, I considered this solution, but I imagined there may be
global variables which has name_like_this in third-party libraries.
So, I'm guessing we need check like this anyway.

> I expected to see a loop here that checked each identifier but I don't see it.
> How does this code check the following?
>   myFunction(int variable1, int another_variable)
>   int variable1, another_variable;
>   int first_variable, secondVariable;
> etc.

Oops, I completely forgot about the latter two cases, and I didn't
implement the first one because this seemed to be a bit more
difficult and this case would be not so common like errors in local
variables. Anyway, I've implemented both cases and added test cases.

> > +        self.assert_lint('while (foo & under_score) {', '')
>
> Ideally this would produce an error.

This test is intended to check a global variables in third-party
library isn't warned. I renamed "under_score" to
"value_in_thirdparty_library" to describe the intention clearly.

> 1. It would be great if this only flagged the first instance of a
> particular identifier (in a given file) to avoid lots of warnings for
> the same identifier and to avoid giving a warning when someone is
> fixing some code and just using a variable that was already in the
> file.

As the function only checks declaration, I think the warnings won't be
so noisy without this.

> 2. Please run this on existing code (if you haven't already) and
> verify that it doesn't give false alarms.

Yes, I'm running the script for WebCore/*/*.{cpp,h} to check this.
This produces 494 warnings now and I don't see any false alarms in the
list.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.