WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
NEW
103157
[style] Add a style-check for enum-member names
https://bugs.webkit.org/show_bug.cgi?id=103157
Summary
[style] Add a style-check for enum-member names
Sadrul Habib Chowdhury
Reported
2012-11-23 11:44:07 PST
[style] Add a style-check for enum-member names
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Sadrul Habib Chowdhury
Comment 1
2012-11-23 11:46:01 PST
Created
attachment 175831
[details]
Patch
Sadrul Habib Chowdhury
Comment 2
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!
Adam Barth
Comment 3
2012-11-24 18:12:13 PST
Unfortunately, I don't know this code very well. Perhaps svn blame can suggest a good reviewer?
Daniel Bates
Comment 4
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 '#'.
Sadrul Habib Chowdhury
Comment 5
2012-11-25 16:03:31 PST
Created
attachment 175893
[details]
Patch
Sadrul Habib Chowdhury
Comment 6
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)
Sadrul Habib Chowdhury
Comment 7
2012-11-25 19:25:11 PST
Created
attachment 175913
[details]
Update for another valid syntax.
Daniel Bates
Comment 8
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()?
Sadrul Habib Chowdhury
Comment 9
2012-11-26 09:45:48 PST
Created
attachment 176022
[details]
Addressed the comments. Thanks!
Sadrul Habib Chowdhury
Comment 10
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())
Daniel Bates
Comment 11
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.
Daniel Bates
Comment 12
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
Daniel Bates
Comment 13
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
Sadrul Habib Chowdhury
Comment 14
2012-11-26 23:45:10 PST
Created
attachment 176188
[details]
Patch
Sadrul Habib Chowdhury
Comment 15
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.
Sadrul Habib Chowdhury
Comment 16
2012-11-26 23:46:20 PST
Comment on
attachment 176188
[details]
Patch Requesting cq?
WebKit Review Bot
Comment 17
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
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug