Bug 103157 – [style] Add a style-check for enum-member names

• Component: New Bugs
• Status: NEW

[Sadrul Habib Chowdhury]

[style] Add a style-check for enum-member names

[Sadrul Habib Chowdhury]

Created attachment 175831 [details]
Patch

[Sadrul Habib Chowdhury]

Hi Adam. The enum naming style bit me a few times (and possibly others, especially devs who primarily work on chromium). So I thought a style-check for this would be useful. Would you be the right reviewer for this? Thanks!

[Adam Barth]

Unfortunately, I don't know this code very well.  Perhaps svn blame can suggest a good reviewer?

[Daniel Bates]

Comment on attachment 175831 [details]
Patch

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

> Tools/Scripts/webkitpy/style/checkers/cpp.py:1219
> +    def process_clean_line(self, line):

Obviously this function doesn't support checking enumerators that are declared on the same line as the enum type:

enum OrdinalNumber { First, Second, Third };

We should consider adding support to check such enums because they appear frequently in the code base.

> Tools/Scripts/webkitpy/style/checkers/cpp.py:1223
> +            elif match(r'^\s*[A-Z_]+,?\s*$', line):

Take line to be "FOO = 6" then this regular expression won't match. That is, this regular expression doesn't match an enumerator-definition with a constant expression.

> Tools/Scripts/webkitpy/style/checkers/cpp.py:1228
> +            if match(r'^\s*enum\s+[a-zA-Z]+\s*\{?\s*$', line):

Did you intend to explicitly exclude checking enums without a tag? For example:
   enum {
       EvenOddWindingRule,
       NonZeroWindingRule
   };

Take line to be "enum Shape3D {". Then the regular expression doesn't match and hence we will not check the enumerators in this enum.

> Tools/Scripts/webkitpy/style/checkers/cpp.py:2072
> +    line = clean_lines.elided[line_number]  # Get rid of comments and strings.

Nit: There are two space characters before the '#'.

[Sadrul Habib Chowdhury]

Created attachment 175893 [details]
Patch

[Sadrul Habib Chowdhury]

Comment on attachment 175831 [details]
Patch

Thanks for the great feedback! Addressed your comments, and updated the test with relevant test cases.

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

>> Tools/Scripts/webkitpy/style/checkers/cpp.py:1219
>> +    def process_clean_line(self, line):
> 
> Obviously this function doesn't support checking enumerators that are declared on the same line as the enum type:
> 
> enum OrdinalNumber { First, Second, Third };
> 
> We should consider adding support to check such enums because they appear frequently in the code base.

Done.

>> Tools/Scripts/webkitpy/style/checkers/cpp.py:1223
>> +            elif match(r'^\s*[A-Z_]+,?\s*$', line):
> 
> Take line to be "FOO = 6" then this regular expression won't match. That is, this regular expression doesn't match an enumerator-definition with a constant expression.

Fixed.

>> Tools/Scripts/webkitpy/style/checkers/cpp.py:1228
>> +            if match(r'^\s*enum\s+[a-zA-Z]+\s*\{?\s*$', line):
> 
> Did you intend to explicitly exclude checking enums without a tag? For example:
>    enum {
>        EvenOddWindingRule,
>        NonZeroWindingRule
>    };
> 
> Take line to be "enum Shape3D {". Then the regular expression doesn't match and hence we will not check the enumerators in this enum.

Fixed.

>> Tools/Scripts/webkitpy/style/checkers/cpp.py:2072
>> +    line = clean_lines.elided[line_number]  # Get rid of comments and strings.
> 
> Nit: There are two space characters before the '#'.

Indeed (this is enforced by a pre-submit check)

[Sadrul Habib Chowdhury]

Created attachment 175913 [details]
Update for another valid syntax.

[Daniel Bates]

Comment on attachment 175913 [details]
Update for another valid syntax.

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

> Tools/Scripts/webkitpy/style/checkers/cpp.py:1222
> +        expr_all_uppercase = r'^\s*[A-Z0-9_]+\s*(=\s*[0-9A-Za-z]+\s*)?,?\s*$'

Take line to be "FOO = 6 * 5" then this regular expression fails to match. Maybe such constant expressions are unlikely?

Nit: From reading <http://docs.python.org/2/library/re.html#re.match>, match() will only return a MatchObject instance when "zero or more characters at the beginning of string match the regular expression pattern"; => the leading caret (^) in the regular expression pattern is superfluous.

> Tools/Scripts/webkitpy/style/checkers/cpp.py:1223
> +        expr_starts_lowercase = r'^\s*[a-z]'

Nit: From reading <http://docs.python.org/2/library/re.html#re.match>, match() will only return a MatchObject instance when "zero or more characters at the beginning of string match the regular expression pattern"; => the leading caret (^) in the regular expression pattern is superfluous.

> Tools/Scripts/webkitpy/style/checkers/cpp.py:1226
> +            if match(r'^\s*' + expr_enum_end + '$', line):

Ditto.

> Tools/Scripts/webkitpy/style/checkers/cpp.py:1233
> +            if match(r'^\s*enum(\s+[a-zA-Z0-9]+)?\s*\{?\s*$', line):

Nit: I suggest that we use a non-capture group for the enum name/tag since we aren't interested in capturing it. See the documentation for (?:...) in <http://docs.python.org/2/library/re.html#regular-expression-syntax> for more details.

> Tools/Scripts/webkitpy/style/checkers/cpp.py:1236
> +                matched = search(r'^\s*enum(\s+[a-zA-Z0-9]+)?\s*\{(?P<members>.*)' + expr_enum_end + '$', line)

Ditto.

How did you come to the decision to use search() instead of match()?

[Sadrul Habib Chowdhury]

Created attachment 176022 [details]
Addressed the comments. Thanks!

[Sadrul Habib Chowdhury]

Comment on attachment 175913 [details]
Update for another valid syntax.

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

>> Tools/Scripts/webkitpy/style/checkers/cpp.py:1222
>> +        expr_all_uppercase = r'^\s*[A-Z0-9_]+\s*(=\s*[0-9A-Za-z]+\s*)?,?\s*$'
> 
> Take line to be "FOO = 6 * 5" then this regular expression fails to match. Maybe such constant expressions are unlikely?
> 
> Nit: From reading <http://docs.python.org/2/library/re.html#re.match>, match() will only return a MatchObject instance when "zero or more characters at the beginning of string match the regular expression pattern"; => the leading caret (^) in the regular expression pattern is superfluous.

I expect such expressions to be unlikely, but I do not really know. I can add certain operators here if you think that'd be useful?

>> Tools/Scripts/webkitpy/style/checkers/cpp.py:1233
>> +            if match(r'^\s*enum(\s+[a-zA-Z0-9]+)?\s*\{?\s*$', line):
> 
> Nit: I suggest that we use a non-capture group for the enum name/tag since we aren't interested in capturing it. See the documentation for (?:...) in <http://docs.python.org/2/library/re.html#regular-expression-syntax> for more details.

Done.

>> Tools/Scripts/webkitpy/style/checkers/cpp.py:1236
>> +                matched = search(r'^\s*enum(\s+[a-zA-Z0-9]+)?\s*\{(?P<members>.*)' + expr_enum_end + '$', line)
> 
> Ditto.
> 
> How did you come to the decision to use search() instead of match()?

Removed all the ^s from the regexes.

I changed this back to using match(). I initially used search() almost arbitrarily (it looked like most of the places in this script that use named groups use search())

[Daniel Bates]

(In reply to comment #10)
> (From update of attachment 175913 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175913&action=review
> 
> >> Tools/Scripts/webkitpy/style/checkers/cpp.py:1222
> >> +        expr_all_uppercase = r'^\s*[A-Z0-9_]+\s*(=\s*[0-9A-Za-z]+\s*)?,?\s*$'
> > 
> > Take line to be "FOO = 6 * 5" then this regular expression fails to match. Maybe such constant expressions are unlikely?
> > 
>> [...]
> 
> I expect such expressions to be unlikely, but I do not really know. I can add certain operators here if you think that'd be useful?

The proposed change is sufficient as is. We can enhance the enum support as needed. For now, I suggest that we add a FIXME comment in process_clean_line() to implement support for all constant expressions.

[Daniel Bates]

Comment on attachment 176022 [details]
Addressed the comments. Thanks!

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

> Tools/Scripts/webkitpy/style/checkers/cpp.py:1222
> +        expr_all_uppercase = r'\s*[A-Z0-9_]+\s*(?:=\s*[0-9A-Za-z]+\s*)?,?\s*$'

I suggest we add a FIXME comment above this line to explain that this regular expression and the regular expression for expr_enum_end only support integers and identifiers for the value of an enumerator, but this is sufficient for our needs at the time of writing (10/26/2012). That is, these regular expressions don't match all constant expressions.

> Tools/Scripts/webkitpy/style/checkers/cpp.py:1224
> +        expr_enum_end = r'}\s*(?:[a-zA-Z0-9]+\s*(?:=\s*[a-zA-Z0-9]+)?)?\s*;\s*'

Nit: For consistency, I suggest that we use the same ordering in characters classes. In particular, the ordering of the second character class in expr_enum_end ([a-zA-Z0-9]) differs from the ordering of second character class in expr_all_uppercase ([0-9A-Za-z]). We should pick an ordering for character classes and be consistent.

> Tools/Scripts/webkitpy/style/checkers/cpp.py:1227
> +            if match(r'\s*' + expr_enum_end + '$', line):

Although not necessary, you may want to consider prefixing the last string literal in this string concatenation with 'r' to have Python treat it as a raw string.

> Tools/Scripts/webkitpy/style/checkers/cpp.py:1234
> +            if match(expr_enum_start + '$', line):

Ditto.

> Tools/Scripts/webkitpy/style/checkers/cpp.py:1237
> +                matched = match(expr_enum_start + '(?P<members>.*)' + expr_enum_end + '$', line)

Although not necessary, you may want to consider prefixing both sting literals '(?P<members>.*)' and '$' with 'r' to have Python treat them as raw strings.

> Tools/Scripts/webkitpy/style/checkers/cpp.py:3548
> +      enum_state: A _EnumState instance which maintains an enum delcaration

Nit: delcaration => declaration

[Daniel Bates]

(In reply to comment #12)
> (From update of attachment 176022 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176022&action=review
> 
> > Tools/Scripts/webkitpy/style/checkers/cpp.py:1222
> > +        expr_all_uppercase = r'\s*[A-Z0-9_]+\s*(?:=\s*[0-9A-Za-z]+\s*)?,?\s*$'
> 
> I suggest we add a FIXME comment above this line...but this is sufficient for our needs at the time of writing (10/26/2012)...

*11/26/2012

[Sadrul Habib Chowdhury]

Created attachment 176188 [details]
Patch

[Sadrul Habib Chowdhury]

Comment on attachment 176022 [details]
Addressed the comments. Thanks!

Addressed all the comments. Thanks!

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

>>> Tools/Scripts/webkitpy/style/checkers/cpp.py:1222
>>> +        expr_all_uppercase = r'\s*[A-Z0-9_]+\s*(?:=\s*[0-9A-Za-z]+\s*)?,?\s*$'
>> 
>> I suggest we add a FIXME comment above this line to explain that this regular expression and the regular expression for expr_enum_end only support integers and identifiers for the value of an enumerator, but this is sufficient for our needs at the time of writing (10/26/2012). That is, these regular expressions don't match all constant expressions.
> 
> *11/26/2012

Done.

>> Tools/Scripts/webkitpy/style/checkers/cpp.py:1224
>> +        expr_enum_end = r'}\s*(?:[a-zA-Z0-9]+\s*(?:=\s*[a-zA-Z0-9]+)?)?\s*;\s*'
> 
> Nit: For consistency, I suggest that we use the same ordering in characters classes. In particular, the ordering of the second character class in expr_enum_end ([a-zA-Z0-9]) differs from the ordering of second character class in expr_all_uppercase ([0-9A-Za-z]). We should pick an ordering for character classes and be consistent.

Done (went with [a-zA-Z0-9] since that seems to be used in a few places in this file)

>> Tools/Scripts/webkitpy/style/checkers/cpp.py:1227
>> +            if match(r'\s*' + expr_enum_end + '$', line):
> 
> Although not necessary, you may want to consider prefixing the last string literal in this string concatenation with 'r' to have Python treat it as a raw string.

Done.

>> Tools/Scripts/webkitpy/style/checkers/cpp.py:1234
>> +            if match(expr_enum_start + '$', line):
> 
> Ditto.

Done.

>> Tools/Scripts/webkitpy/style/checkers/cpp.py:1237
>> +                matched = match(expr_enum_start + '(?P<members>.*)' + expr_enum_end + '$', line)
> 
> Although not necessary, you may want to consider prefixing both sting literals '(?P<members>.*)' and '$' with 'r' to have Python treat them as raw strings.

Done.

>> Tools/Scripts/webkitpy/style/checkers/cpp.py:3548
>> +      enum_state: A _EnumState instance which maintains an enum delcaration
> 
> Nit: delcaration => declaration

Fixed.

[Sadrul Habib Chowdhury]

Comment on attachment 176188 [details]
Patch

Requesting cq?

[WebKit Review Bot]

Comment on attachment 176188 [details]
Patch

Clearing flags on attachment: 176188

Committed r135832: <http://trac.webkit.org/changeset/135832>