[webkit-reviews] review granted: [Bug 32051] check-webkit-style should check for camelCase variable names : [Attachment 44252] Patch v3

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has granted Shinichiro Hamaji
<hamaji at chromium.org>'s request for review:
Bug 32051: check-webkit-style should check for camelCase variable names
https://bugs.webkit.org/show_bug.cgi?id=32051

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

------- Additional Comments from David Levin <levin at chromium.org>
Looks good. I've noted a few things that you may want to consider changing when
landing but none are critical.

> diff --git a/WebKitTools/Scripts/modules/cpp_style.py
b/WebKitTools/Scripts/modules/cpp_style.py
> +def check_identifier_name_in_declaration(filename, line_number, line,
error):
> +    """Checks if identifier names contain any underscores.
> +
> +    As identifiers in libraries we are using have a bunch of
> +    underscores, we only warn the declarations of identifiers and

s/warn/warn about/


> +	 error: The function to call with any errors found.
> +    """
> +    if match('\s*return', line):

Should there be a \b after return?

> +    # Convert "long long", "long double", and "long long int" to
> +    # simple types, but don't remove simple "long".
> +    line = sub(r'long (?:long )?(?=long|double|int)', '', line)

I'm not sure why this uses the non-group form of parenthesis (and the same
comment goes for other places especially now that the code uses named groups).
(The code doesn't need the group, but putting in the ?: just introduces more
visual noise for me at least.)

> +    line = sub(r'^\s*for\s*\(', '', line)
> +    line, control_statement = subn(r'^\s*(?:while|else if|if|switch)\s*\(',
'', line)

I think qt uses foreach as well but it is probably fine to leave it as
unsupported for now.