Bug 105832 – [style] WebIDL-reflecting upper-case enums reported as style violation

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Zan Dobersek]

[style] WebIDL-reflecting upper-case enums reported as style violation

[Zan Dobersek]

Created attachment 180875 [details]
Patch

[Adam Barth]

Interesting idea.  Who is the best person to review this change?

[Tony Chang]

Comment on attachment 180875 [details]
Patch

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

When are you seeing this? I thought the generated files weren't checked in so the style checker doesn't run on them.

> Tools/Scripts/webkitpy/style/checkers/cpp.py:1219
> +        self.webidl_enum = False

Nit: I think is_webidl_enum would be clearer.

> Tools/Scripts/webkitpy/style/checkers/cpp.py:2098
> +    if match(r'\s*// Web(?:Kit)?IDL enum\s*$', clean_lines.raw_lines[line_number]):
> +        enum_state.webidl_enum = True

I would probably do:
  enum_state.is_webidl_enum = True if match(r'\s*// Web(?:Kit)?IDL enum\s*$', clean_lines.raw_lines[line_number]) else False

Also, it seems more common to compile regular expressions and store them as globals.

[Zan Dobersek]

(In reply to comment #3)
> (From update of attachment 180875 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180875&action=review
> 
> When are you seeing this? I thought the generated files weren't checked in so the style checker doesn't run on them.

This is the case with enums in WebCore, for instance in Source/WebCore/Modules/webaudio/PannerNode.h:
http://trac.webkit.org/browser/trunk/Source/WebCore/Modules/webaudio/PannerNode.h#L49

These still get checked, but are reported as violations while they're basically only reflecting IDL and are required to be upper-cased (while C++ enums are expected to be InterCaps).

[Zan Dobersek]

Comment on attachment 180875 [details]
Patch

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

>> Tools/Scripts/webkitpy/style/checkers/cpp.py:2098
>> +        enum_state.webidl_enum = True
> 
> I would probably do:
>   enum_state.is_webidl_enum = True if match(r'\s*// Web(?:Kit)?IDL enum\s*$', clean_lines.raw_lines[line_number]) else False
> 
> Also, it seems more common to compile regular expressions and store them as globals.

Addressing the last line, many one-time expressions are written in-line and are then compiled and cached by match, sub, search and other similar methods. Simply justifying the current approach, I can still change it if you have a strong opinion about it.

[Zan Dobersek]

Created attachment 181157 [details]
Patch

[Tony Chang]

Comment on attachment 181157 [details]
Patch

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

> Tools/Scripts/webkitpy/style/checkers/cpp.py:2097
> +    enum_state.is_webidl_enum = bool(match(r'\s*// Web(?:Kit)?IDL enum\s*$', clean_lines.raw_lines[line_number])) if not enum_state.is_webidl_enum else True

Nit: I would probably do:
enum_state.is_webidl_enum |= bool(match(r'\s*// Web(?:Kit)?IDL enum\s*$', clean_lines.raw_lines[line_number]))

[Zan Dobersek]

(In reply to comment #7)
> (From update of attachment 181157 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181157&action=review
> 
> > Tools/Scripts/webkitpy/style/checkers/cpp.py:2097
> > +    enum_state.is_webidl_enum = bool(match(r'\s*// Web(?:Kit)?IDL enum\s*$', clean_lines.raw_lines[line_number])) if not enum_state.is_webidl_enum else True
> 
> Nit: I would probably do:
> enum_state.is_webidl_enum |= bool(match(r'\s*// Web(?:Kit)?IDL enum\s*$', clean_lines.raw_lines[line_number]))

Looks better. Adjusted and committed as r138719.
http://trac.webkit.org/changeset/138719