[Webkit-unassigned] [Bug 25884] WebKit needs a style linting tool
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jul 13 22:31:27 PDT 2009
https://bugs.webkit.org/show_bug.cgi?id=25884
David Levin <levin at chromium.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #32660|review? |review-
Flag| |
--- Comment #33 from David Levin <levin at chromium.org> 2009-07-13 22:31:24 PDT ---
(From update of attachment 32660)
Ok, here's my detailed review. After you address these issues, I think we can
land this. Thanks for your patience.
Throughout change TODO to FIXME (and get rid of any assignment after the TODO).
i.e. "TODO(levin):" becomes "FIXME:"
> diff --git a/WebKitTools/Scripts/cpplint.py b/WebKitTools/Scripts/cpplint.py
> +_ERROR_CATEGORIES = '''\
> + build/class
These are still indented by 2 spaces. Although it is in quotes, might as well
make it 4 to match.
> +# We used to check for high-bit characters, but after much discussion we
> +# decided those were OK, as long as they were in UTF-8 and didn't represent
> +# hard-coded international strings, which belong in a seperate i18n file.
Another Google specific comment that doesn't apply to WebKit.
> +for op, replacement in [('==', 'EQ'), ('!=', 'NE'),
> + ('>=', 'GE'), ('>', 'GT'),
> + ('<=', 'LE'), ('<', 'LT')]:
> + _CHECK_REPLACEMENT['DCHECK'][op] = 'DCHECK_%s' % replacement
> + _CHECK_REPLACEMENT['CHECK'][op] = 'CHECK_%s' % replacement
> + _CHECK_REPLACEMENT['EXPECT_TRUE'][op] = 'EXPECT_%s' % replacement
> + _CHECK_REPLACEMENT['ASSERT_TRUE'][op] = 'ASSERT_%s' % replacement
> + _CHECK_REPLACEMENT['EXPECT_TRUE_M'][op] = 'EXPECT_%s_M' % replacement
> + _CHECK_REPLACEMENT['ASSERT_TRUE_M'][op] = 'ASSERT_%s_M' % replacement
Two space indent.
> +
> +for op, inv_replacement in [('==', 'NE'), ('!=', 'EQ'),
> + ('>=', 'LT'), ('>', 'LE'),
> + ('<=', 'GT'), ('<', 'GE')]:
> + _CHECK_REPLACEMENT['EXPECT_FALSE'][op] = 'EXPECT_%s' % inv_replacement
> + _CHECK_REPLACEMENT['ASSERT_FALSE'][op] = 'ASSERT_%s' % inv_replacement
> + _CHECK_REPLACEMENT['EXPECT_FALSE_M'][op] = 'EXPECT_%s_M' % inv_replacement
> + _CHECK_REPLACEMENT['ASSERT_FALSE_M'][op] = 'ASSERT_%s_M' % inv_replacement
Two space indent.
> +def _output_format():
> + """Gets the module's output format."""
> + return _cpplint_state.output_format
> +
> +
A single line to separate the functions is sufficient (though two lines before
a class of course).
> +def find_next_multi_line_comment_start(lines, lineix):
> + """Find the beginning marker for a multiline comment."""
> + while lineix < len(lines):
s/lineix/line_index/
> +
> +
> +def close_expression(clean_lines, linenum, pos):
s/lineum/line_number/
> + line = clean_lines.elided[linenum]
> + startchar = line[pos]
s/startchar/start_character/
> + if startchar not in '({[':
> + return (line, clean_lines.num_lines(), -1)
> + if startchar == '(': endchar = ')'
s/endchar/end_character/
> + if startchar == '[': endchar = ']'
> + if startchar == '{': endchar = '}'
Put the statements on separate lines
if startchar == '(':
endchar = ')'
etc.
> +def check_for_copyright(filename, lines, error):
> + """Logs an error if no Copyright message appears at the top of the file."""
> +
> + # We'll say it should occur by line 10. Don't forget there's a
> + # dummy line at the front.
> + for line in xrange(1, min(len(lines), 11)):
> + if re.search(r'Copyright', lines[line], re.I): break
Put the statements on separate lines
if re.search(r'Copyright', lines[line], re.I):
break
> + cppvar = get_header_guard_cpp_variable(filename)
> +
> + ifndef = None
> + ifndef_linenum = 0
s/ifndef_linenum/ifndef_line_number/
> + define = None
> + endif = None
> + endif_linenum = 0
s/endif_linenum/endif_line_number/
> + for linenum, line in enumerate(lines):
s/linenum/line_number/ -- throughout this file.
> + linesplit = line.split()
s/linesplit/line_split/
> +threading_list = (
THREADING_LIST = (
Since it is a constant.
> + for single_thread_function, multithread_safe_function in threading_list:
> + ix = line.find(single_thread_function)
s/ix/index/
> + if search(r'(\w+|[+-]?\d+(\.\d*)?)\s*(<|>)\?=?\s*(\w+|[+-]?\d+)(\.\d*)?',
> + line):
I would unwrap this line.
> + if (args and
> + args.group(1) != 'void' and
> + not match(r'(const\s+)?%s\s*&' % re.escape(base_classname),
> + args.group(1).strip())):
"and" placement. -- Let's follow standard WebKit style and put the "and/or" at
the beginning of the continuation line as opposed to the end of the line.
For example,
if (args
and args.group(1) != 'void'
and not match(r'(const\s+)?%s\s*&' % re.escape(base_classname),
args.group(1).strip())):
> + if ((classinfo.virtual_method_linenumber is not None) and
> + (not classinfo.has_virtual_destructor) and
> + (not classinfo.is_derived)): # Only warn for base classes
"and" placement.
> + if ( # Ignore control structures.
> + not search(r'\b(if|for|while|switch|return|new|delete)\b', fncall) and
> + # Ignore pointers/references to functions.
> + not search(r' \([^)]+\)\([^)]*(\)|,$)', fncall) and
> + # Ignore pointers/references to arrays.
> + not search(r' \([^)]+\)\[[^\]]+\]', fncall)):
"and" placement.
> + if function_name == 'TEST' or function_name == 'TEST_F' or (
> + not match(r'[A-Z_]+$', function_name)):
I would just unwrap this line. (WebKit doesn't stop code at 80 columns.)
> + # Next, we complain if there's a comment too near the text
> + commentpos = line.find('//')
s/commentpos/comment_position/
> + if commentpos != -1:
> + # Check if the // may be in quotes. If so, ignore it
> + # Comparisons made explicit for clarity -- pylint: disable-msg=C6403
> + if (line.count('"', 0, commentpos) -
> + line.count('\\"', 0, commentpos)) % 2 == 0: # not in quotes
I would unwrap this.
> + # Allow one space for new scopes, two spaces otherwise:
> + if (not match(r'^\s*{ //', line) and
> + ((commentpos >= 1 and
> + line[commentpos-1] not in string.whitespace) or
> + (commentpos >= 2 and
> + line[commentpos-2] not in string.whitespace))):
"and/or" placment
> + matched = (search(r'[=/-]{4,}\s*$', line[commentend:]) or
> + search(r'^/+ ', line[commentend:]))
"or" placement
> + matched = search(r'\b(if|for|while|switch)\s*'
> + r'\(([ ]*)(.).*[^ ]+([ ]*)\)\s*{\s*$',
> + line)
These two lines should be indented slightly.
> + if matched:
> + if len(matched.group(2)) != len(matched.group(4)):
> + if not (matched.group(3) == ';' and
> + len(matched.group(2)) == 1 + len(matched.group(4)) or
> + not matched.group(2) and search(r'\bfor\s*\(.*; \)', line)):
"and/or" placement.
> + elif (search(r'\s+;\s*$', line) and
> + not search(r'\bfor\b', line)):
"and" placement.
> + elif (search(r'\b(for|while)\s*\(.*\)\s*;\s*$', line) and
> + line.count('(') == line.count(')') and
> + # Allow do {} while();
> + not search(r'}\s*while', line)):
"and" placement.
> + prevlinenum = linenum - 1
s/prevlinenum/previous_line_number/
> + prevline = get_previous_non_blank_line(clean_lines, linenum)[0]
> + if (not search(r'[;:}{)=]\s*$|\)\s*const\s*$', prevline) and
> + prevline.find('#') < 0):
"and" placement.
> + if (search(r'{.*}\s*;', line) and
> + line.count('{') == line.count('}') and
> + not search(r'struct|class|enum|\s*=\s*{', line)):
"and" placement.
> +def get_line_width(line):
> + if isinstance(line, unicode):
> + width = 0
> + for c in unicodedata.normalize('NFC', line):
> + if unicodedata.east_asian_width(c) in ('W', 'F'):
> + width += 2
> + elif not unicodedata.combining(c):
> + width += 1
> + return width
> + else:
> + return len(line)
Since the "if" ends with a "return" then is no need for the else (following
standard WebKit code style).
> +def check_style(filename, clean_lines, linenum, file_extension, error):
> + """Checks rules from the 'C++ style rules' section of cppguide.html.
> +
> + Most of these rules are hard to test (naming, comment style), but we
> + do what we can. In particular we check for 2-space indents, line lengths,
s/2-space indents/4-space indents/
> + # One or three blank spaces at the beginning of the line is weird; it's
> + # hard to reconcile that with 2-space indents.
s/2-space indents/4-space indents/
> + # Labels should always be indented at least one space.
> + elif not initial_spaces and line[:2] != '//' and search(r'[^:]:\s*$',
> + line):
I would unwrap the "line):" which looks like it was done to meet the 80 column
limit (which WebKit doesn't have).
> +
> + # Check if the line is a header guard.
> + is_header_guard = False
> + if file_extension == 'h':
> + cppvar = get_header_guard_cpp_variable(filename)
> + if (line.startswith('#ifndef %s' % cppvar) or
> + line.startswith('#define %s' % cppvar) or
> + line.startswith('#endif // %s' % cppvar)):
> + is_header_guard = True
is_header_guard is unused now so you can remove this bit of code.
> +
> + if (cleansed_line.count(';') > 1 and
> + # for loops are allowed two ;'s (and may run over two lines).
> + cleansed_line.find('for') == -1 and
> + (get_previous_non_blank_line(clean_lines, linenum)[0].find('for') == -1 or
> + get_previous_non_blank_line(clean_lines, linenum)[0].find(';') != -1) and
> + # It's ok to have many commands in a switch case that fits in 1 line
> + not ((cleansed_line.find('case ') != -1 or
> + cleansed_line.find('default:') != -1) and
> + cleansed_line.find('break;') != -1)):
"and/or" placement.
> + for suffix in ('test.cc', 'regtest.cc', 'unittest.cc',
> + 'inl.h', 'impl.h', 'internal.h'):
> + if (filename.endswith(suffix) and len(filename) > len(suffix) and
> + filename[-len(suffix) - 1] in ('-', '_')):
> + return filename[:-len(suffix) - 1]
"and" placement.
> + if (filename.endswith('_test.cc') or
> + filename.endswith('_unittest.cc') or
> + filename.endswith('_regtest.cc')):
"or" placement.
> + return True
> + else:
> + return False
Remove "else" since the if has a return.
> + if is_system:
> + if is_cpp_h:
> + return _CPP_SYS_HEADER
> + else:
> + return _C_SYS_HEADER
Remove "else" since the if has a return.
> +
> + if target_base == include_base and (
> + include_dir == target_dir or
> + include_dir == os.path.normpath(target_dir + '/../public')):
"and/or" placement.
> + if (target_first_component and include_first_component and
> + target_first_component.group(0) ==
> + include_first_component.group(0)):
"and" placement.
Also, having to do
"a ==
b"
is just ugly so I'd unwrap this part of it.
> + if linenum + 1 < clean_lines.num_lines():
> + extended_line = line + clean_lines.elided[linenum + 1]
> + else:
> + extended_line = line
extended_line is unused.
> + # Make Windows paths like Unix.
> + fullname = os.path.abspath(filename).replace('\\', '/')
fullname is unused.
> +def use_webkit_styles():
> + global _DEFAULT_FILTERS
> + _DEFAULT_FILTERS = [
> + '-whitespace/end_of_line',
> + '-whitespace/comments',
I don't think the whitespace/comments is correct for WebKit right now. WebKit
style uses 1 space before end of line comments (unliks Google style which does
2 spaces), so I'd just remove this one until we fix it.
> +def main():
> + use_webkit_styles()
As I've looked through this code, I really want it to get landed but I'm
concerned there are issues with respect to WebKit style that will have to be
fixed after it is landed when multiple people can contribute and we can do
incremental changes to improve it. Given that it is a tool and not part of
this shipping code, I think that it is ok to handle in this fashion.
However, it would be nice to let people know about the potential alpha state of
the warnings (due to the adaption to WebKit style).
In short for now, it would be nice to print a warning banner here that this
tool is still under development and may flag things incorrectly. After it is
landed and people have an opportunity to try it out we can remove the warning
banner.
Something like this:
WARNING WARNING WARNING
This tool is in the process of development and may give inaccurate results at
present.
Please files bugs (and/or patches) for things that you notice that it flags
incorrectly.
WARNING WARNING WARNING
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list