Bug 105832 - [style] WebIDL-reflecting upper-case enums reported as style violation
Summary: [style] WebIDL-reflecting upper-case enums reported as style violation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-12-28 07:47 PST by Zan Dobersek
Modified: 2013-01-03 10:52 PST (History)
5 users (show)

See Also:


Attachments
Patch (4.90 KB, patch)
2012-12-28 07:55 PST, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (4.86 KB, patch)
2013-01-03 01:31 PST, Zan Dobersek
tony: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2012-12-28 07:47:53 PST
[style] WebIDL-reflecting upper-case enums reported as style violation
Comment 1 Zan Dobersek 2012-12-28 07:55:06 PST
Created attachment 180875 [details]
Patch
Comment 2 Adam Barth 2013-01-02 11:19:35 PST
Interesting idea.  Who is the best person to review this change?
Comment 3 Tony Chang 2013-01-02 11:33:49 PST
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.
Comment 4 Zan Dobersek 2013-01-03 00:58:10 PST
(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).
Comment 5 Zan Dobersek 2013-01-03 01:12:01 PST
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.
Comment 6 Zan Dobersek 2013-01-03 01:31:07 PST
Created attachment 181157 [details]
Patch
Comment 7 Tony Chang 2013-01-03 10:01:39 PST
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]))
Comment 8 Zan Dobersek 2013-01-03 10:45:28 PST
(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