Summary: | Stylechecker: False positive on inline asm code. | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Gergő Balogh <gbalogh.u-szeged> | ||||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | commit-queue, galpeter, gbalogh.u-szeged, glenn, llango.u-szeged, ossy, rniwa | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Gergő Balogh
2014-03-21 01:29:06 PDT
Created attachment 227398 [details]
patch
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 Created attachment 228822 [details]
patch fix
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. Created attachment 228825 [details]
patch fix fix
(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). Created attachment 228838 [details]
patch fix
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. Created attachment 228939 [details]
patch fix
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 Committed r167010: <http://trac.webkit.org/changeset/167010> |