[webkit-reviews] review denied: [Bug 32051] check-webkit-style should check for camelCase variable names : [Attachment 44149] Patch v1

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied 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 44149: Patch v1
https://bugs.webkit.org/attachment.cgi?id=44149&action=review

------- Additional Comments from David Levin <levin at chromium.org>
> diff --git a/WebKitTools/Scripts/modules/cpp_style.py
b/WebKitTools/Scripts/modules/cpp_style.py

> +def check_identifier_name(filename, line_number, line, error):
> +    """Checks if identifier names don't contain an underscore.
How about"
  Checks if identifier names contain any underscores.

> +    # Remove storage sepcifiers, ...

typo: sepcifiers

Instead of saying "what is the code doing?" in the comment. It would be good to
answer "why?" (When I read the code, I don't understand why this is necessary.)


> +    line =
re.sub(r'(?:unsigned|signed|inline|using|static|const|volatile|auto|register|ex
tern|typedef|else|restrict|struct|class|virtual|return) ', '', line)
> +
> +    # Remove all template parameters by removing matching < and >.

Why?

> +    while True:
> +	   line, removed_numbers = re.subn(r'<[\w\s*&:]+>', '', line)

s/removed_numbers/number_of_replacements/

> +	   if not removed_numbers:
> +	       break
> +
> +    # Remove for statement.
Why?

> +    line = re.sub(r'^\s*for\s*\(', '', line)
> +    # Remove other control statements.
> +    line, control_statement = re.subn(r'^\s*(?:while|if|switch)\s*\(', '',
line)

Why?

> +    # Detect variable and functions.
> +    matched = match(r'\s*\w(?:[\w:]|\s*[*&])+\s+([\w:]+)\s*([;(=[])', line)

This regex is hard to read as is. I don't know what it is attempting to do with
all of its clauses. One way to fix this is to make parts of the part into
separate strings that are put in variables which have names explaining what
they do.

> +    if not matched:
> +	   return
> +    # If we removed a non-for-control statement, the character after
> +    # the identifier should be '='. With this rule, we can avoid the
> +    # case like "if (val & INT_MAX) {".

What are you trying to avoid?  Flagging INT_MAX?

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


> +    if control_statement and matched.group(2) != '=':
> +	   return
> +
> +    identifier = matched.group(1)

It would be nice to use named groups.

> +    # Remove "m_" and "s_" to alow them.

typo: alow

> +    modified_identifier = re.sub(r'(?:^|(?<=::))[ms]_', '', identifier)
> +    if modified_identifier.find('_') >= 0:
> +	   error(filename, line_number, 'readability/naming', 4, identifier + "
is unallowed naming. Don't use underscores in your identifier names.")

s/unallowed naming/incorrectly named/

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.

> diff --git a/WebKitTools/Scripts/modules/cpp_style_unittest.py
b/WebKitTools/Scripts/modules/cpp_style_unittest.py

> +	   self.assert_lint('short _length;',
> +			    '_length' + name_error_message)

What about a test for length_ as well?

> +	   self.assert_lint('const int UNDER_SCORE;',
> +			    'UNDER_SCORE' + name_error_message)

Given how simple the regex matching is. I think that allowing NAMES_LIKE_THIS
is fine. People rarely make that mistake and it is a valid name for a #define'd
item.


> +	   self.assert_lint('static inline const char const & const
under_score;',

& is in the wrong place.

> +	   self.assert_lint('WTF::Vector<WTF::RefPtr<const RenderObject *
const> > under_score;',

The * is in the wrong place.

> +	   self.assert_lint('while (foo & under_score) {', '')

Ideally this would produce an error.