[SCM] WebKit Debian packaging branch, debian/experimental, updated. debian/1.3.8-1-1049-g2e11a8e
levin at chromium.org
levin at chromium.org
Fri Jan 21 14:43:20 UTC 2011
The following commit has been merged in the debian/experimental branch:
commit 364f1bb367ccae0ed6dcf360ef676f3edcb7e48f
Author: levin at chromium.org <levin at chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Date: Mon Dec 27 18:50:54 2010 +0000
check-webkit-style check for meaningless variable names in function declarations.
https://bugs.webkit.org/show_bug.cgi?id=51523
Reviewed by Eric Seidel.
* Scripts/webkitpy/style/checker.py: Exempted JavaScriptCore/jit/JITStubs.cpp
from the new check and whitespace/parens because the syntax is unusual and
produced a fair number of positives for these checks.
* Scripts/webkitpy/style/checkers/cpp.py:
(_convert_to_lower_with_underscores): Used as a canonical form for type names
and parameter names when determining if the parameter name is useless.
(_create_acronym): Used to check for redundant variable names in cases like "ExceptionCode ec"
(Parameter.lower_with_underscores_name): Gives back the parameter name in a lower_with_underscore
format.
(_check_parameter_name_against_text): Checks to see if the parameter name is in the
text or an acronym of it.
(check_function_definition): Checks function definitions for meaningless variable names.
(process_line): Added call to check_function_definition.
* Scripts/webkitpy/style/checkers/cpp_unittest.py:
(CppFunctionsTest.test_convert_to_lower_with_underscores): Test for _convert_to_lower_with_underscores.
(CppFunctionsTest.test_create_acronym): Test for _create_acronym.
(CppFunctionsTest.test_check_parameter_against_text): Test for _check_parameter_against_text.
(CppStyleTestBase.perform_single_line_lint): Removed the parameter name check
because when only checking a snippet, there are a lot of bogus functions.
(CppStyleTestBase.perform_multi_line_lint): Ditto and removed a bogus filter
that I put there previously and just noticed.
(WebKitStyleTest.test_parameter_names): Tests for the functionality -- both
check_function_definition and process_line.
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@74688 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Tools/ChangeLog b/Tools/ChangeLog
index cbf5af9..d50caba 100644
--- a/Tools/ChangeLog
+++ b/Tools/ChangeLog
@@ -1,3 +1,34 @@
+2010-12-27 David Levin <levin at chromium.org>
+
+ Reviewed by Eric Seidel.
+
+ check-webkit-style check for meaningless variable names in function declarations.
+ https://bugs.webkit.org/show_bug.cgi?id=51523
+
+ * Scripts/webkitpy/style/checker.py: Exempted JavaScriptCore/jit/JITStubs.cpp
+ from the new check and whitespace/parens because the syntax is unusual and
+ produced a fair number of positives for these checks.
+ * Scripts/webkitpy/style/checkers/cpp.py:
+ (_convert_to_lower_with_underscores): Used as a canonical form for type names
+ and parameter names when determining if the parameter name is useless.
+ (_create_acronym): Used to check for redundant variable names in cases like "ExceptionCode ec"
+ (Parameter.lower_with_underscores_name): Gives back the parameter name in a lower_with_underscore
+ format.
+ (_check_parameter_name_against_text): Checks to see if the parameter name is in the
+ text or an acronym of it.
+ (check_function_definition): Checks function definitions for meaningless variable names.
+ (process_line): Added call to check_function_definition.
+ * Scripts/webkitpy/style/checkers/cpp_unittest.py:
+ (CppFunctionsTest.test_convert_to_lower_with_underscores): Test for _convert_to_lower_with_underscores.
+ (CppFunctionsTest.test_create_acronym): Test for _create_acronym.
+ (CppFunctionsTest.test_check_parameter_against_text): Test for _check_parameter_against_text.
+ (CppStyleTestBase.perform_single_line_lint): Removed the parameter name check
+ because when only checking a snippet, there are a lot of bogus functions.
+ (CppStyleTestBase.perform_multi_line_lint): Ditto and removed a bogus filter
+ that I put there previously and just noticed.
+ (WebKitStyleTest.test_parameter_names): Tests for the functionality -- both
+ check_function_definition and process_line.
+
2010-12-27 Carlos Garcia Campos <cgarcia at igalia.com>
Reviewed by Martin Robinson.
diff --git a/Tools/Scripts/webkitpy/style/checker.py b/Tools/Scripts/webkitpy/style/checker.py
index ba6502c..4bcd84a 100644
--- a/Tools/Scripts/webkitpy/style/checker.py
+++ b/Tools/Scripts/webkitpy/style/checker.py
@@ -156,6 +156,10 @@ _PATH_RULES_SPECIFIER = [
# we don't check for underscores in that directory.
"/JavaScriptCore/assembler/"],
["-readability/naming"]),
+ ([# JITStubs has an usual syntax which causes false alarms for a few checks.
+ "JavaScriptCore/jit/JITStubs.cpp"],
+ ["-readability/parameter_name",
+ "-whitespace/parens"]),
# WebKit2 rules:
# WebKit2 doesn't use config.h, and certain directories have other
diff --git a/Tools/Scripts/webkitpy/style/checkers/cpp.py b/Tools/Scripts/webkitpy/style/checkers/cpp.py
index f721b9f..902a0a1 100644
--- a/Tools/Scripts/webkitpy/style/checkers/cpp.py
+++ b/Tools/Scripts/webkitpy/style/checkers/cpp.py
@@ -47,6 +47,8 @@ import string
import sys
import unicodedata
+from webkitpy.common.memoized import memoized
+
# The key to use to provide a class to fake loading a header file.
INCLUDE_IO_INJECTION_KEY = 'include_header_io'
@@ -192,6 +194,34 @@ def iteratively_replace_matches_with_char(pattern, char_replacement, s):
s = s[:start_match_index] + char_replacement * match_length + s[end_match_index:]
+def _convert_to_lower_with_underscores(text):
+ """Converts all text strings in camelCase or PascalCase to lowers with underscores."""
+
+ # First add underscores before any capital letter followed by a lower case letter
+ # as long as it is in a word.
+ # (This put an underscore before Password but not P and A in WPAPassword).
+ text = sub(r'(?<=[A-Za-z0-9])([A-Z])(?=[a-z])', r'_\1', text)
+
+ # Next add underscores before capitals at the end of words if it was
+ # preceeded by lower case letter or number.
+ # (This puts an underscore before A in isA but not A in CBA).
+ text = sub(r'(?<=[a-z0-9])([A-Z])(?=\b)', r'_\1', text)
+
+ # Next add underscores when you have a captial letter which is followed by a capital letter
+ # but is not proceeded by one. (This puts an underscore before A in 'WordADay').
+ text = sub(r'(?<=[a-z0-9])([A-Z][A-Z_])', r'_\1', text)
+
+ return text.lower()
+
+
+
+def _create_acronym(text):
+ """Creates an acronym for the given text."""
+ # Removes all lower case letters except those starting words.
+ text = sub(r'(?<!\b)[a-z]', '', text)
+ return text.upper()
+
+
def up_to_unmatched_closing_paren(s):
"""Splits a string into two parts up to first unmatched ')'.
@@ -323,6 +353,11 @@ class Parameter(object):
self.name = sub(r'=.*', '', parameter[parameter_name_index:]).strip()
self.row = row
+ @memoized
+ def lower_with_underscores_name(self):
+ """Returns the parameter name in the lower with underscores format."""
+ return _convert_to_lower_with_underscores(self.name)
+
class SingleLineView(object):
"""Converts multiple lines into a single line (with line breaks replaced by a
@@ -1443,6 +1478,71 @@ def check_for_function_lengths(clean_lines, line_number, function_state, error):
function_state.count(line_number) # Count non-blank/non-comment lines.
+def _check_parameter_name_against_text(parameter, text, error):
+ """Checks to see if the parameter name is contained within the text.
+
+ Return false if the check failed (i.e. an error was produced).
+ """
+
+ # Treat 'lower with underscores' as a canonical form because it is
+ # case insensitive while still retaining word breaks. (This ensures that
+ # 'elate' doesn't look like it is duplicating of 'NateLate'.)
+ canonical_parameter_name = parameter.lower_with_underscores_name()
+
+ # Appends "object" to all text to catch variables that did the same (but only
+ # do this when the parameter name is more than a single character to avoid
+ # flagging 'b' which may be an ok variable when used in an rgba function).
+ if len(canonical_parameter_name) > 1:
+ text = sub(r'(\w)\b', r'\1Object', text)
+ canonical_text = _convert_to_lower_with_underscores(text)
+
+ # Used to detect cases like ec for ExceptionCode.
+ acronym = _create_acronym(text).lower()
+ if canonical_text.find(canonical_parameter_name) != -1 or acronym.find(canonical_parameter_name) != -1:
+ error(parameter.row, 'readability/parameter_name', 5,
+ 'The parameter name "%s" adds no information, so it should be removed.' % parameter.name)
+ return False
+ return True
+
+
+def check_function_definition(clean_lines, line_number, function_state, error):
+ """Check that function definitions for style issues.
+
+ Specifically, check that parameter names in declarations add information.
+
+ Args:
+ clean_lines: A CleansedLines instance containing the file.
+ line_number: The number of the line to check.
+ function_state: Current function name and lines in body so far.
+ error: The function to call with any errors found.
+ """
+ # Only do checks when we have a function declaration.
+ if line_number != function_state.body_start_line_number or not function_state.is_declaration:
+ return
+
+ parameter_list = function_state.parameter_list()
+ for parameter in parameter_list:
+ if not parameter.name:
+ continue
+
+ # Check the parameter name against the function name for single parameter set functions.
+ if len(parameter_list) == 1 and match('set[A-Z]', function_state.current_function):
+ trimmed_function_name = function_state.current_function[len('set'):]
+ if not _check_parameter_name_against_text(parameter, trimmed_function_name, error):
+ # Since an error was noted for this name, move to the next parameter.
+ continue
+
+ # Check the parameter name against the type.
+ if not _check_parameter_name_against_text(parameter, parameter.type, error):
+ continue
+
+ # Skip single letter parameters before comparing against value (because 'a' would
+ # be flagged, but it may be an ok variable when used in an rgba function).
+ if len(parameter.name) > 1:
+ # 'value' is a meaningless variable name that is used in some places, so flag it.
+ _check_parameter_name_against_text(parameter, 'value', error)
+
+
def check_pass_ptr_usage(clean_lines, line_number, function_state, error):
"""Check for proper usage of Pass*Ptr.
@@ -3156,6 +3256,7 @@ def process_line(filename, file_extension,
check_for_function_lengths(clean_lines, line, function_state, error)
if search(r'\bNOLINT\b', raw_lines[line]): # ignore nolint lines
return
+ check_function_definition(clean_lines, line, function_state, error)
check_pass_ptr_usage(clean_lines, line, function_state, error)
check_for_multiline_comments_and_strings(clean_lines, line, error)
check_style(clean_lines, line, file_extension, class_state, file_state, error)
@@ -3239,6 +3340,7 @@ class CppChecker(object):
'readability/function',
'readability/multiline_comment',
'readability/multiline_string',
+ 'readability/parameter_name',
'readability/naming',
'readability/null',
'readability/pass_ptr',
diff --git a/Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py b/Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py
index 4f84693..8ee73fe 100644
--- a/Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py
+++ b/Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py
@@ -109,6 +109,19 @@ class CppFunctionsTest(unittest.TestCase):
"""Supports testing functions that do not need CppStyleTestBase."""
+ def test_convert_to_lower_with_underscores(self):
+ self.assertEquals(cpp_style._convert_to_lower_with_underscores('ABC'), 'abc')
+ self.assertEquals(cpp_style._convert_to_lower_with_underscores('aB'), 'a_b')
+ self.assertEquals(cpp_style._convert_to_lower_with_underscores('isAName'), 'is_a_name')
+ self.assertEquals(cpp_style._convert_to_lower_with_underscores('AnotherTest'), 'another_test')
+ self.assertEquals(cpp_style._convert_to_lower_with_underscores('PassRefPtr<MyClass>'), 'pass_ref_ptr<my_class>')
+ self.assertEquals(cpp_style._convert_to_lower_with_underscores('_ABC'), '_abc')
+
+ def test_create_acronym(self):
+ self.assertEquals(cpp_style._create_acronym('ABC'), 'ABC')
+ self.assertEquals(cpp_style._create_acronym('IsAName'), 'IAN')
+ self.assertEquals(cpp_style._create_acronym('PassRefPtr<MyClass>'), 'PRP<MC>')
+
def test_is_c_or_objective_c(self):
clean_lines = cpp_style.CleansedLines([''])
clean_objc_lines = cpp_style.CleansedLines(['#import "header.h"'])
@@ -206,6 +219,12 @@ class CppFunctionsTest(unittest.TestCase):
index += 1
self.assertEquals(index, len(expected_parameters))
+ def test_check_parameter_against_text(self):
+ error_collector = ErrorCollector(self.assert_)
+ parameter = cpp_style.Parameter('FooF ooF', 4, 1)
+ self.assertFalse(cpp_style._check_parameter_name_against_text(parameter, 'FooF', error_collector))
+ self.assertEquals(error_collector.results(),
+ 'The parameter name "ooF" adds no information, so it should be removed. [readability/parameter_name] [5]')
class CppStyleTestBase(unittest.TestCase):
"""Provides some useful helper functions for cpp_style tests.
@@ -239,6 +258,7 @@ class CppStyleTestBase(unittest.TestCase):
basic_error_rules = ('-build/header_guard',
'-legal/copyright',
'-readability/fn_size',
+ '-readability/parameter_name',
'-whitespace/ending_newline')
return self.perform_lint(code, filename, basic_error_rules)
@@ -246,7 +266,7 @@ class CppStyleTestBase(unittest.TestCase):
def perform_multi_line_lint(self, code, file_extension):
basic_error_rules = ('-build/header_guard',
'-legal/copyright',
- '-multi_line_filter',
+ '-readability/parameter_name',
'-whitespace/ending_newline')
return self.perform_lint(code, 'test.' + file_extension, basic_error_rules)
@@ -4098,6 +4118,59 @@ class WebKitStyleTest(CppStyleTestBase):
self.assert_lint('OwnPtr<uint32_t> under_score(new uint32_t);',
'under_score' + name_underscore_error_message)
+ def test_parameter_names(self):
+ # Leave meaningless variable names out of function declarations.
+ meaningless_variable_name_error_message = 'The parameter name "%s" adds no information, so it should be removed. [readability/parameter_name] [5]'
+
+ parameter_error_rules = ('-',
+ '+readability/parameter_name')
+ # No variable name, so no error.
+ self.assertEquals('',
+ self.perform_lint('void func(int);', 'test.cpp', parameter_error_rules))
+
+ # Verify that copying the name of the set function causes the error (with some odd casing).
+ self.assertEquals(meaningless_variable_name_error_message % 'itemCount',
+ self.perform_lint('void setItemCount(size_t itemCount);', 'test.cpp', parameter_error_rules))
+ self.assertEquals(meaningless_variable_name_error_message % 'abcCount',
+ self.perform_lint('void setABCCount(size_t abcCount);', 'test.cpp', parameter_error_rules))
+
+ # Verify that copying a type name will trigger the warning (even if the type is a template parameter).
+ self.assertEquals(meaningless_variable_name_error_message % 'context',
+ self.perform_lint('void funct(PassRefPtr<ScriptExecutionContext> context);', 'test.cpp', parameter_error_rules))
+
+ # Verify that acronyms as variable names trigger the error (for both set functions and type names).
+ self.assertEquals(meaningless_variable_name_error_message % 'ec',
+ self.perform_lint('void setExceptionCode(int ec);', 'test.cpp', parameter_error_rules))
+ self.assertEquals(meaningless_variable_name_error_message % 'ec',
+ self.perform_lint('void funct(ExceptionCode ec);', 'test.cpp', parameter_error_rules))
+
+ # 'object' alone, appended, or as part of an acronym is meaningless.
+ self.assertEquals(meaningless_variable_name_error_message % 'object',
+ self.perform_lint('void funct(RenderView object);', 'test.cpp', parameter_error_rules))
+ self.assertEquals(meaningless_variable_name_error_message % 'viewObject',
+ self.perform_lint('void funct(RenderView viewObject);', 'test.cpp', parameter_error_rules))
+ self.assertEquals(meaningless_variable_name_error_message % 'rvo',
+ self.perform_lint('void funct(RenderView rvo);', 'test.cpp', parameter_error_rules))
+
+ # 'value' is a meaningless variable name.
+ self.assertEquals(meaningless_variable_name_error_message % 'value',
+ self.perform_lint('void funct(RenderView value);', 'test.cpp', parameter_error_rules))
+
+ # Check that r, g, b, and a are allowed.
+ self.assertEquals('',
+ self.perform_lint('void setRGBAValues(int r, int g, int b, int a);', 'test.cpp', parameter_error_rules))
+
+ # Verify that a simple substring match isn't done which would cause false positives.
+ self.assertEquals('',
+ self.perform_lint('void setNateLateCount(size_t elate);', 'test.cpp', parameter_error_rules))
+ self.assertEquals('',
+ self.perform_lint('void funct(NateLate elate);', 'test.cpp', parameter_error_rules))
+
+ # Don't have generate warnings for functions (only declarations).
+ self.assertEquals('',
+ self.perform_lint('void funct(PassRefPtr<ScriptExecutionContext> context)\n'
+ '{\n'
+ '}\n', 'test.cpp', parameter_error_rules))
def test_comments(self):
# A comment at the beginning of a line is ok.
--
WebKit Debian packaging
More information about the Pkg-webkit-commits
mailing list