[webkit-reviews] review denied: [Bug 130570] Stylechecker: False positive on inline asm code. : [Attachment 227398] patch

[bugzilla-daemon at webkit.org]

Csaba Osztrogonác <ossy at webkit.org> has denied Gergő Balogh
<gbalogh.u-szeged at partner.samsung.com>'s request for review:
Bug 130570: Stylechecker: False positive on inline asm code.
https://bugs.webkit.org/show_bug.cgi?id=130570

Attachment 227398: patch
https://bugs.webkit.org/attachment.cgi?id=227398&action=review

------- Additional Comments from Csaba Osztrogonác <ossy at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=227398&action=review


The idea is good to disable style checking inside inline assembly blocks, but
we can do it little bit better.

> Tools/ChangeLog:7
> +	   Stylechecker: False positive on inline asm code.
> +	   https://bugs.webkit.org/show_bug.cgi?id=130570
> +
> +	   Reviewed by NOBODY (OOPS!).
> +

Please add a comment that you disable style checking at all for inline assembly
blocks.

> Tools/Scripts/webkitpy/style/checkers/cpp.py:3616
>      if match(r'\s*\b__asm\b', raw_lines[line]):  # Ignore asm lines as they
format differently.
>	   return
> +    asm_state.process_line(raw_lines[line])

We should avoid all check below, with this early return if we are in asm block.


> Tools/Scripts/webkitpy/style/checkers/cpp.py:3621
> +    check_style(clean_lines, line, file_extension, class_state, file_state,
enum_state, asm_state, error)

Passing asm_state here is unnecessary because of the early return.
Consequently we don't need any change in check_style and
check_member_initialization_list.

> Tools/Scripts/webkitpy/style/checkers/cpp.py:3632
> +	   super(_InlineASMState, self).__init__()

we don't need this

> Tools/Scripts/webkitpy/style/checkers/cpp.py:3636
> +	   if match(r'\s*asm\s+volatile\(', line):

volatile should be optional

> Tools/Scripts/webkitpy/style/checkers/cpp.py:3640
> +	   pass

we don't need this pass here

> Tools/Scripts/webkitpy/style/checkers/cpp.py:3642
> +    def isInside(self):

isInside -> is_in_asm