Bug 103157 - [style] Add a style-check for enum-member names
Summary: [style] Add a style-check for enum-member names
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sadrul Habib Chowdhury
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-23 11:44 PST by Sadrul Habib Chowdhury
Modified: 2017-07-18 08:29 PDT (History)
7 users (show)

See Also:


Attachments
Patch (8.77 KB, patch)
2012-11-23 11:46 PST, Sadrul Habib Chowdhury
no flags Details | Formatted Diff | Diff
Patch (9.66 KB, patch)
2012-11-25 16:03 PST, Sadrul Habib Chowdhury
no flags Details | Formatted Diff | Diff
Update for another valid syntax. (9.84 KB, patch)
2012-11-25 19:25 PST, Sadrul Habib Chowdhury
no flags Details | Formatted Diff | Diff
Addressed the comments. Thanks! (9.88 KB, patch)
2012-11-26 09:45 PST, Sadrul Habib Chowdhury
dbates: review+
Details | Formatted Diff | Diff
Patch (10.15 KB, patch)
2012-11-26 23:45 PST, Sadrul Habib Chowdhury
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sadrul Habib Chowdhury 2012-11-23 11:44:07 PST
[style] Add a style-check for enum-member names
Comment 1 Sadrul Habib Chowdhury 2012-11-23 11:46:01 PST
Created attachment 175831 [details]
Patch
Comment 2 Sadrul Habib Chowdhury 2012-11-23 11:48:36 PST
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!
Comment 3 Adam Barth 2012-11-24 18:12:13 PST
Unfortunately, I don't know this code very well.  Perhaps svn blame can suggest a good reviewer?
Comment 4 Daniel Bates 2012-11-24 20:26:50 PST
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 '#'.
Comment 5 Sadrul Habib Chowdhury 2012-11-25 16:03:31 PST
Created attachment 175893 [details]
Patch
Comment 6 Sadrul Habib Chowdhury 2012-11-25 16:06:12 PST
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)
Comment 7 Sadrul Habib Chowdhury 2012-11-25 19:25:11 PST
Created attachment 175913 [details]
Update for another valid syntax.
Comment 8 Daniel Bates 2012-11-25 20:18:41 PST
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()?
Comment 9 Sadrul Habib Chowdhury 2012-11-26 09:45:48 PST
Created attachment 176022 [details]
Addressed the comments. Thanks!
Comment 10 Sadrul Habib Chowdhury 2012-11-26 09:46:25 PST
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())
Comment 11 Daniel Bates 2012-11-26 22:32:32 PST
(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.
Comment 12 Daniel Bates 2012-11-26 22:33:31 PST
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
Comment 13 Daniel Bates 2012-11-26 22:37:38 PST
(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
Comment 14 Sadrul Habib Chowdhury 2012-11-26 23:45:10 PST
Created attachment 176188 [details]
Patch
Comment 15 Sadrul Habib Chowdhury 2012-11-26 23:45:53 PST
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.
Comment 16 Sadrul Habib Chowdhury 2012-11-26 23:46:20 PST
Comment on attachment 176188 [details]
Patch

Requesting cq?
Comment 17 WebKit Review Bot 2012-11-27 00:39:42 PST
Comment on attachment 176188 [details]
Patch

Clearing flags on attachment: 176188

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