| Summary: | [style] Add a style-check for enum-member names | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Sadrul Habib Chowdhury <sadrul> | ||||||||||||
| Component: | New Bugs | Assignee: | Sadrul Habib Chowdhury <sadrul> | ||||||||||||
| Status: | NEW --- | ||||||||||||||
| Severity: | Normal | CC: | abarth, dbates, dpranke, eric, levin, tkent, webkit.review.bot | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Sadrul Habib Chowdhury
2012-11-23 11:44:07 PST
Created attachment 175831 [details]
Patch
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! Unfortunately, I don't know this code very well. Perhaps svn blame can suggest a good reviewer? 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 '#'. Created attachment 175893 [details]
Patch
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) Created attachment 175913 [details]
Update for another valid syntax.
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()? Created attachment 176022 [details]
Addressed the comments. Thanks!
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()) (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 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 (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 Created attachment 176188 [details]
Patch
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 on attachment 176188 [details]
Patch
Requesting cq?
Comment on attachment 176188 [details] Patch Clearing flags on attachment: 176188 Committed r135832: <http://trac.webkit.org/changeset/135832> |