Changeset 53251 in webkit


Ignore:
Timestamp:
Jan 14, 2010 2:19:13 AM (14 years ago)
Author:
eric@webkit.org
Message:

2010-01-14 Chris Jerdonek <chris.jerdonek@gmail.com>

Reviewed by Shinichiro Hamaji.

Moved error() from cpp_style.py to checker.py.

https://bugs.webkit.org/show_bug.cgi?id=33620

  • Scripts/check-webkit-style:
    • Addressed FIXME to not set global state.
  • Scripts/webkitpy/style/checker.py:
    • Added argument validation to ProcessorOptions constructor.
    • Added should_report_error() to ProcessorOptions class.
    • Removed set_options().
    • Added StyleChecker class.
  • Scripts/webkitpy/style/checker_unittest.py:
    • Added unit test class for ProcessorOptions class.
    • Added unit test to check that parse() strips white space.
  • Scripts/webkitpy/style/cpp_style.py:
    • Removed "filter" and "output_format" methods.
    • Removed should_print_error() and error() functions.
    • Removed default parameter value from process_file().
  • Scripts/webkitpy/style/cpp_style_unittest.py:
    • Removed call to cpp_style._should_print_error().
    • Removed test_filter() and test_filter_appending().
  • Scripts/webkitpy/style/text_style.py:
    • Removed default parameter value from process_file().
Location:
trunk/WebKitTools
Files:
7 edited

Legend:

Unmodified
Added
Removed
  • trunk/WebKitTools/ChangeLog

    r53249 r53251  
     12010-01-14  Chris Jerdonek  <chris.jerdonek@gmail.com>
     2
     3        Reviewed by Shinichiro Hamaji.
     4
     5        Moved error() from cpp_style.py to checker.py.
     6
     7        https://bugs.webkit.org/show_bug.cgi?id=33620
     8
     9        * Scripts/check-webkit-style:
     10          - Addressed FIXME to not set global state.
     11
     12        * Scripts/webkitpy/style/checker.py:
     13          - Added argument validation to ProcessorOptions constructor.
     14          - Added should_report_error() to ProcessorOptions class.
     15          - Removed set_options().
     16          - Added StyleChecker class.
     17
     18        * Scripts/webkitpy/style/checker_unittest.py:
     19          - Added unit test class for ProcessorOptions class.
     20          - Added unit test to check that parse() strips white space.
     21
     22        * Scripts/webkitpy/style/cpp_style.py:
     23          - Removed "filter" and "output_format" methods.
     24          - Removed should_print_error() and error() functions.
     25          - Removed default parameter value from process_file().
     26
     27        * Scripts/webkitpy/style/cpp_style_unittest.py:
     28          - Removed call to cpp_style._should_print_error().
     29          - Removed test_filter() and test_filter_appending().
     30
     31        * Scripts/webkitpy/style/text_style.py:
     32          - Removed default parameter value from process_file().
     33
    1342010-01-14  Diego Gonzalez  <diego.gonzalez@openbossa.org>
    235
  • trunk/WebKitTools/Scripts/check-webkit-style

    r52906 r53251  
    6767    (files, options) = parser.parse(sys.argv[1:])
    6868
    69     # FIXME: Eliminate the need to call this function.
    70     #        Options should be passed into process_file instead.
    71     checker.set_options(options)
     69    styleChecker = checker.StyleChecker(options)
    7270
    7371    if files:
    7472        for filename in files:
    75             checker.process_file(filename)
     73            styleChecker.process_file(filename)
    7674
    7775    else:
     
    8987        else:
    9088            patch = scm.create_patch()
    91         checker.process_patch(patch)
     89        styleChecker.process_patch(patch)
    9290
    9391    sys.stderr.write('Total errors found: %d\n' % checker.error_count())
  • trunk/WebKitTools/Scripts/webkitpy/style/checker.py

    r53185 r53251  
    306306                     and "vs7" which Microsoft Visual Studio 7 can parse.
    307307
    308       verbosity: An integer 1-5 that restricts output to errors with a
    309                  confidence score at or above this value.
     308      verbosity: An integer between 1-5 inclusive that restricts output
     309                 to errors with a confidence score at or above this value.
    310310                 The default is 1, which displays all errors.
    311311
     
    319319                         pairs that are not otherwise represented by this
    320320                         class. The default is the empty dictionary.
     321
    321322    """
    322323
    323     def __init__(self, output_format, verbosity=1, filter=None,
     324    def __init__(self, output_format="emacs", verbosity=1, filter=None,
    324325                 git_commit=None, extra_flag_values=None):
    325326        if filter is None:
     
    328329            extra_flag_values = {}
    329330
     331        if output_format not in ("emacs", "vs7"):
     332            raise ValueError('Invalid "output_format" parameter: '
     333                             'value must be "emacs" or "vs7". '
     334                             'Value given: "%s".' % output_format)
     335
     336        if (verbosity < 1) or (verbosity > 5):
     337            raise ValueError('Invalid "verbosity" parameter: '
     338                             "value must be an integer between 1-5 inclusive. "
     339                             'Value given: "%s".' % verbosity)
     340
    330341        self.output_format = output_format
    331342        self.verbosity = verbosity
     
    334345        self.extra_flag_values = extra_flag_values
    335346
    336 
    337 # FIXME: Eliminate the need for this function.
    338 #        Options should be passed into process_file instead.
    339 def set_options(options):
    340     """Initialize global _CppStyleState instance.
    341 
    342     This needs to be called before calling process_file.
    343 
    344     Args:
    345       options: A ProcessorOptions instance.
    346     """
    347     cpp_style._set_output_format(options.output_format)
    348     cpp_style._set_verbose_level(options.verbosity)
    349     cpp_style._set_filter(options.filter)
     347    def should_report_error(self, category, confidence_in_error):
     348        """Return whether an error should be reported.
     349
     350        An error should be reported if the confidence in the error
     351        is at least the current verbosity level, and if the current
     352        filter says that the category should be checked.
     353
     354        Args:
     355          category: A string that is a style category.
     356          confidence_in_error: An integer between 1 and 5, inclusive, that
     357                               represents the application's confidence in
     358                               the error. A higher number signifies greater
     359                               confidence.
     360
     361        """
     362        if confidence_in_error < self.verbosity:
     363            return False
     364
     365        if self.filter is None:
     366            return True # All categories should be checked by default.
     367
     368        return self.filter.should_check(category)
    350369
    351370
     
    437456        Args:
    438457          error_message: A string that is an error message to print.
     458
    439459        """
    440460        usage = self.create_usage(self.defaults)
     
    552572
    553573        verbosity = int(verbosity)
    554         if ((verbosity < 1) or (verbosity > 5)):
     574        if (verbosity < 1) or (verbosity > 5):
    555575            raise ValueError('Invalid --verbose value %s: value must '
    556576                             'be between 1-5.' % verbosity)
     
    564584
    565585
    566 def process_file(filename):
    567     """Checks style for the specified file.
    568 
    569     If the specified filename is '-', applies cpp_style to the standard input.
    570     """
    571     if cpp_style.can_handle(filename) or filename == '-':
    572         cpp_style.process_file(filename)
    573     elif text_style.can_handle(filename):
    574         text_style.process_file(filename)
    575 
    576 
    577 def process_patch(patch_string):
    578     """Does lint on a single patch.
    579 
    580     Args:
    581       patch_string: A string of a patch.
    582     """
    583     patch = DiffParser(patch_string.splitlines())
    584     for filename, diff in patch.files.iteritems():
    585         file_extension = os.path.splitext(filename)[1]
    586         line_numbers = set()
    587 
    588         def error_for_patch(filename, line_number, category, confidence, message):
    589             """Wrapper function of cpp_style.error for patches.
    590 
    591             This function outputs errors only if the line number
    592             corresponds to lines which are modified or added.
    593             """
    594             if not line_numbers:
    595                 for line in diff.lines:
    596                     # When deleted line is not set, it means that
    597                     # the line is newly added.
    598                     if not line[0]:
    599                         line_numbers.add(line[1])
    600 
    601             if line_number in line_numbers:
    602                 cpp_style.error(filename, line_number, category, confidence, message)
    603 
    604         if cpp_style.can_handle(filename):
    605             cpp_style.process_file(filename, error=error_for_patch)
     586class StyleChecker(object):
     587
     588    """Supports checking style in files and patches."""
     589
     590    def __init__(self, options):
     591        """Create a StyleChecker instance.
     592
     593        Args:
     594          options: A ProcessorOptions instance.
     595
     596        """
     597        self.options = options
     598
     599        # FIXME: Eliminate the need to set global state here.
     600        cpp_style._set_verbose_level(options.verbosity)
     601
     602    def _handle_error(self, filename, line_number, category, confidence, message):
     603        """Logs the fact we've found a lint error.
     604
     605        We log the error location and our confidence in the error, i.e.
     606        how certain we are the error is a legitimate style regression
     607        versus a misidentification or justified use.
     608
     609        Args:
     610          filename: The name of the file containing the error.
     611          line_number: The number of the line containing the error.
     612          category: A string used to describe the "category" this bug
     613                    falls under: "whitespace", say, or "runtime".
     614                    Categories may have a hierarchy separated by slashes:
     615                    "whitespace/indent".
     616          confidence: A number from 1-5 representing a confidence score
     617                      for the error, with 5 meaning that we are certain
     618                      of the problem, and 1 meaning that it could be a
     619                      legitimate construct.
     620          message: The error message.
     621
     622        """
     623        if not self.options.should_report_error(category, confidence):
     624            return
     625
     626        # FIXME: Eliminate the need to reference cpp_style here.
     627        cpp_style._cpp_style_state.increment_error_count()
     628
     629        if self.options.output_format == 'vs7':
     630            sys.stderr.write('%s(%s):  %s  [%s] [%d]\n' % (
     631                filename, line_number, message, category, confidence))
     632        else:
     633            sys.stderr.write('%s:%s:  %s  [%s] [%d]\n' % (
     634                filename, line_number, message, category, confidence))
     635
     636    def process_file(self, filename):
     637        """Checks style for the specified file.
     638
     639        If the specified filename is '-', applies cpp_style to the standard input.
     640
     641        """
     642        if cpp_style.can_handle(filename) or filename == '-':
     643            cpp_style.process_file(filename, self._handle_error)
    606644        elif text_style.can_handle(filename):
    607             text_style.process_file(filename, error=error_for_patch)
     645            text_style.process_file(filename, self._handle_error)
     646
     647    def process_patch(self, patch_string):
     648        """Does lint on a single patch.
     649
     650        Args:
     651          patch_string: A string of a patch.
     652
     653        """
     654        patch = DiffParser(patch_string.splitlines())
     655        for filename, diff in patch.files.iteritems():
     656            file_extension = os.path.splitext(filename)[1]
     657            line_numbers = set()
     658
     659            def error_for_patch(filename, line_number, category, confidence, message):
     660                """Wrapper function of cpp_style.error for patches.
     661
     662                This function outputs errors only if the line number
     663                corresponds to lines which are modified or added.
     664                """
     665                if not line_numbers:
     666                    for line in diff.lines:
     667                        # When deleted line is not set, it means that
     668                        # the line is newly added.
     669                        if not line[0]:
     670                            line_numbers.add(line[1])
     671
     672                if line_number in line_numbers:
     673                    self._handle_error(filename, line_number, category, confidence, message)
     674
     675            if cpp_style.can_handle(filename):
     676                cpp_style.process_file(filename, error_for_patch)
     677            elif text_style.can_handle(filename):
     678                text_style.process_file(filename, error_for_patch)
    608679
    609680
  • trunk/WebKitTools/Scripts/webkitpy/style/checker_unittest.py

    r53186 r53251  
    3939import checker as style
    4040from checker import CategoryFilter
    41 
     41from checker import ProcessorOptions
    4242
    4343class CategoryFilterTest(unittest.TestCase):
     
    8484        self.assertFalse(filter.should_check("abc"))
    8585        self.assertTrue(filter.should_check("a"))
     86
     87
     88class ProcessorOptionsTest(unittest.TestCase):
     89
     90    """Tests ProcessorOptions class."""
     91
     92    def test_init(self):
     93        """Test __init__ constructor."""
     94        # Check default parameters.
     95        options = ProcessorOptions()
     96        self.assertEquals(options.extra_flag_values, {})
     97        self.assertEquals(options.filter, CategoryFilter([]))
     98        self.assertEquals(options.git_commit, None)
     99        self.assertEquals(options.output_format, "emacs")
     100        self.assertEquals(options.verbosity, 1)
     101
     102        self.assertRaises(ValueError, ProcessorOptions, output_format="bad")
     103        ProcessorOptions(output_format="emacs") # No ValueError: works
     104        ProcessorOptions(output_format="vs7") # works
     105        self.assertRaises(ValueError, ProcessorOptions, verbosity=0)
     106        self.assertRaises(ValueError, ProcessorOptions, verbosity=6)
     107        ProcessorOptions(verbosity=1) # works
     108        ProcessorOptions(verbosity=5) # works
     109
     110        # Check attributes.
     111        options = ProcessorOptions(extra_flag_values={"extra_value" : 2},
     112                                   filter=CategoryFilter(["+"]),
     113                                   git_commit="commit",
     114                                   output_format="vs7",
     115                                   verbosity=3)
     116        self.assertEquals(options.extra_flag_values, {"extra_value" : 2})
     117        self.assertEquals(options.filter, CategoryFilter(["+"]))
     118        self.assertEquals(options.git_commit, "commit")
     119        self.assertEquals(options.output_format, "vs7")
     120        self.assertEquals(options.verbosity, 3)
     121
     122    def test_should_report_error(self):
     123        """Test should_report_error()."""
     124        filter = CategoryFilter(["-xyz"])
     125        options = ProcessorOptions(filter=filter, verbosity=3)
     126
     127        # Test verbosity
     128        self.assertTrue(options.should_report_error("abc", 3))
     129        self.assertFalse(options.should_report_error("abc", 2))
     130
     131        # Test filter
     132        self.assertTrue(options.should_report_error("xy", 3))
     133        self.assertFalse(options.should_report_error("xyz", 3))
    86134
    87135
     
    240288        self.assertEquals(options.filter,
    241289                          CategoryFilter(["-", "+whitespace", "+foo", "-bar"]))
     290        # Spurious white space in filter rules.
     291        (files, options) = parse(['--filter=+foo ,-bar'])
     292        self.assertEquals(options.filter,
     293                          CategoryFilter(["-", "+whitespace", "+foo", "-bar"]))
    242294
    243295        # Pass extra flag values.
  • trunk/WebKitTools/Scripts/webkitpy/style/cpp_style.py

    r53185 r53251  
    249249        self.verbose_level = 1  # global setting.
    250250        self.error_count = 0    # global count of reported errors
    251         # filter to apply when emitting error messages
    252         self.filter = None
    253 
    254         # output format:
    255         # "emacs" - format that emacs can parse (default)
    256         # "vs7" - format that Microsoft Visual Studio 7 can parse
    257         self.output_format = 'emacs'
    258 
    259     def set_output_format(self, output_format):
    260         """Sets the output format for errors."""
    261         self.output_format = output_format
    262251
    263252    def set_verbose_level(self, level):
     
    267256        return last_verbose_level
    268257
    269     def set_filter(self, filter):
    270         """Sets the error-message filter.
    271 
    272         Args:
    273           filter: A CategoryFilter instance.
    274 
    275         """
    276         self.filter = filter
    277 
    278258    def reset_error_count(self):
    279259        """Sets the module's error statistic back to zero."""
     
    288268
    289269
    290 def _output_format():
    291     """Gets the module's output format."""
    292     return _cpp_style_state.output_format
    293 
    294 
    295 def _set_output_format(output_format):
    296     """Sets the module's output format."""
    297     _cpp_style_state.set_output_format(output_format)
    298 
    299 
    300270def _verbose_level():
    301271    """Returns the module's verbosity setting."""
     
    306276    """Sets the module's verbosity, and returns the previous setting."""
    307277    return _cpp_style_state.set_verbose_level(level)
    308 
    309 
    310 def _filter():
    311     """Returns the module's CategoryFilter instance."""
    312     return _cpp_style_state.filter
    313 
    314 
    315 def _set_filter(filter):
    316     """Sets the module's error-message filter.
    317 
    318     The filter is applied when deciding whether to emit a given
    319     error message.
    320 
    321     Args:
    322       filter: A CategoryFilter instance.
    323 
    324     """
    325     _cpp_style_state.set_filter(filter)
    326278
    327279
     
    476428        """File has a source file extension."""
    477429        return self.extension()[1:] in ('c', 'cc', 'cpp', 'cxx')
    478 
    479 
    480 def _should_print_error(category, confidence):
    481     """Returns true iff confidence >= verbose, and category passes filter."""
    482     # There are two ways we might decide not to print an error message:
    483     # the verbosity level isn't high enough, or the filters filter it out.
    484     if confidence < _cpp_style_state.verbose_level:
    485         return False
    486 
    487     filter = _filter()
    488 
    489     if filter is None:
    490         return True # All categories should be checked by default.
    491 
    492     return filter.should_check(category)
    493 
    494 
    495 def error(filename, line_number, category, confidence, message):
    496     """Logs the fact we've found a lint error.
    497 
    498     We log where the error was found, and also our confidence in the error,
    499     that is, how certain we are this is a legitimate style regression, and
    500     not a misidentification or a use that's sometimes justified.
    501 
    502     Args:
    503       filename: The name of the file containing the error.
    504       line_number: The number of the line containing the error.
    505       category: A string used to describe the "category" this bug
    506                 falls under: "whitespace", say, or "runtime".  Categories
    507                 may have a hierarchy separated by slashes: "whitespace/indent".
    508       confidence: A number from 1-5 representing a confidence score for
    509                   the error, with 5 meaning that we are certain of the problem,
    510                   and 1 meaning that it could be a legitimate construct.
    511       message: The error message.
    512     """
    513     # There are two ways we might decide not to print an error message:
    514     # the verbosity level isn't high enough, or the filters filter it out.
    515     if _should_print_error(category, confidence):
    516         _cpp_style_state.increment_error_count()
    517         if _cpp_style_state.output_format == 'vs7':
    518             sys.stderr.write('%s(%s):  %s  [%s] [%d]\n' % (
    519                 filename, line_number, message, category, confidence))
    520         else:
    521             sys.stderr.write('%s:%s:  %s  [%s] [%d]\n' % (
    522                 filename, line_number, message, category, confidence))
    523430
    524431
     
    29702877
    29712878
    2972 def process_file(filename, error=error):
     2879def process_file(filename, error):
    29732880    """Performs cpp_style on a single file.
    29742881
  • trunk/WebKitTools/Scripts/webkitpy/style/cpp_style_unittest.py

    r53185 r53251  
    4646#        suggestion on how to best do this.
    4747from checker import STYLE_CATEGORIES
    48 from checker import CategoryFilter
    4948
    5049# This class works as an error collector and replaces cpp_style.Error
     
    6766                        ' which is not in STYLE_CATEGORIES' % (message, category))
    6867        self._seen_style_categories[category] = 1
    69         if cpp_style._should_print_error(category, confidence):
    70             self._errors.append('%s  [%s] [%d]' % (message, category, confidence))
     68        self._errors.append('%s  [%s] [%d]' % (message, category, confidence))
    7169
    7270    def results(self):
     
    15721570                         'Tab found; better to use spaces  [whitespace/tab] [1]')
    15731571
    1574     def test_filter(self):
    1575         old_filter = cpp_style._cpp_style_state.filter
    1576         try:
    1577             cpp_style._cpp_style_state.set_filter(CategoryFilter(['-', '+whitespace', '-whitespace/indent']))
    1578             self.assert_lint(
    1579                 '// Hello there ',
    1580                 'Line ends in whitespace.  Consider deleting these extra spaces.'
    1581                 '  [whitespace/end_of_line] [4]')
    1582             self.assert_lint('int a = (int)1.0;', '')
    1583             self.assert_lint(' weird opening space', '')
    1584         finally:
    1585             cpp_style._cpp_style_state.filter = old_filter
    1586 
    1587     def test_filter_appending(self):
    1588         old_filter = cpp_style._cpp_style_state.filter
    1589         try:
    1590             # Reset filters
    1591             cpp_style._cpp_style_state.set_filter(CategoryFilter(['-whitespace']))
    1592             self.assert_lint('// Hello there ', '')
    1593             cpp_style._cpp_style_state.set_filter(CategoryFilter(['-whitespace', '+whitespace/end_of_line']))
    1594             self.assert_lint(
    1595                 '// Hello there ',
    1596                 'Line ends in whitespace.  Consider deleting these extra spaces.'
    1597                 '  [whitespace/end_of_line] [4]')
    1598             self.assert_lint(' weird opening space', '')
    1599         finally:
    1600             cpp_style._cpp_style_state.filter = old_filter
    1601 
    16021572    def test_unnamed_namespaces_in_headers(self):
    16031573        self.assert_language_rules_check(
  • trunk/WebKitTools/Scripts/webkitpy/style/text_style.py

    r52906 r53251  
    5252
    5353
    54 def process_file(filename, error=cpp_style.error):
     54def process_file(filename, error):
    5555    """Performs lint check for text on a single file."""
    5656    if (not can_handle(filename)):
Note: See TracChangeset for help on using the changeset viewer.