Bug 130570 – Stylechecker: False positive on inline asm code.

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

[Gergő Balogh]

The style check identifies some part of the asm code as an initialization list.
Noticed in the patch https://bugs.webkit.org/show_bug.cgi?id=130500

[Gergő Balogh]

Created attachment 227398 [details]
patch

[Csaba Osztrogonác]

Comment on attachment 227398 [details]
patch

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

[Gergő Balogh]

Created attachment 228822 [details]
patch fix

[Peter Gal]

Comment on attachment 228822 [details]
patch fix

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

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

This is still present.

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

Why use {0,1} here? why not a simple '?' ? That would achieve the same and a bit more descriptive on what we want.

[Gergő Balogh]

Created attachment 228825 [details]
patch fix fix

[Peter Gal]

(In reply to comment #5)
> Created an attachment (id=228825) [details]
> patch fix fix

The patch does not apply, because I think you've edited it manually. Please don't do such things if you don't know how to do it. (You've removed a line and didn't updated the line number calculations).

[Gergő Balogh]

Created attachment 228838 [details]
patch fix

[Csaba Osztrogonác]

Comment on attachment 228838 [details]
patch fix

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

Now it looks much better, r=me.

> Tools/Scripts/webkitpy/style/checkers/cpp.py:2021
> -    if search(r'^(?P<indentation>\s*)((explicit\s+)?[^(\s|\?)]+\([^\?]*\)\s?\:|^(\s|\?)*\:)([^\:]|\Z)[^;]*$', line):
> +    is_list_start = search(r'^(?P<indentation>\s*)((explicit\s+)?[^(\s|\?)]+\([^\?]*\)\s?\:|^(\s|\?)*\:)([^\:]|\Z)[^;]*$', line)
> +    if is_list_start:

just a nit: do we really need this change? I don't think so.
Please change it back before landing.

[Gergő Balogh]

Created attachment 228939 [details]
patch fix

[Csaba Osztrogonác]

Comment on attachment 228939 [details]
patch fix

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

r=me

> Tools/Scripts/webkitpy/style/checkers/cpp.py:3653
> +    if asm_state.is_in_asm():  # Ignore further checks because asm blocks formated differently.

typo: formated -> formatted

[Csaba Osztrogonác]

Committed r167010: <http://trac.webkit.org/changeset/167010>