Bug 49513 - check-webkit-style should detect PassRefPtr usage in functions.
Summary: check-webkit-style should detect PassRefPtr usage in functions.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: David Levin
URL:
Keywords:
Depends on:
Blocks: 49519
  Show dependency treegraph
 
Reported: 2010-11-14 13:22 PST by David Levin
Modified: 2010-11-14 17:17 PST (History)
4 users (show)

See Also:


Attachments
Patch (7.52 KB, patch)
2010-11-14 13:26 PST, David Levin
no flags Details | Formatted Diff | Diff
Patch (7.92 KB, patch)
2010-11-14 16:43 PST, David Levin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Levin 2010-11-14 13:22:48 PST
This is a semi-common mistake for contributors so it would be good to detect it automatically.
Comment 1 David Levin 2010-11-14 13:26:28 PST
Created attachment 73856 [details]
Patch
Comment 2 Daniel Bates 2010-11-14 15:43:42 PST
Comment on attachment 73856 [details]
Patch

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

> WebKitTools/Scripts/webkitpy/style/checkers/cpp.py:1227
> -    """Reports for issues related to functions.
> +    """Reports for long function bodies.

For completeness,  this comment was recently changed in <http://trac.webkit.org/changeset/71986> from "Reports for long function bodies"  to "Reports for issues related to functions". So, this patch is reverting to "Reports for long function bodies", which is more descriptive of the purpose of this function.

> WebKitTools/Scripts/webkitpy/style/checkers/cpp_unittest.py:227
> +    # Similar to perform_function_lengths_check, but calls check_pass_ptr_usage
> +    # instead of check_for_function_lengths.
> +    def perform_pass_ptr_check(self, code):
> +        error_collector = ErrorCollector(self.assert_)
> +        function_state = cpp_style._FunctionState(self.min_confidence)
> +        lines = code.split('\n')
> +        cpp_style.remove_multi_line_comments(lines, error_collector)
> +        lines = cpp_style.CleansedLines(lines)
> +        for i in xrange(lines.num_lines()):
> +            cpp_style.detect_functions(lines, i,
> +                                       function_state, error_collector)
> +            cpp_style.check_pass_ptr_usage(lines, i,
> +                                           function_state, error_collector)
> +        return error_collector.results()

This makes me sad :-(. As you pointed out in the comment, this code is almost identical to the code in CppStyleTestBase.perform_function_lengths_check().

Moreover, this code (or similarly structured code) seems to appear many times throughout cpp_unittest.py, including CppStyleTestBase.perform_single_line_lint(), CppStyleTestBase.perform_multi_line_lint(), CppStyleTestBase.perform_language_rules_check(), CppStyleTestBase.perform_function_lengths_check(), and CppStyleTestBase.perform_include_what_you_use(). It also shares a similar structure to CppChecker._process_lines().

Because this code appears in so many places we should try to factor out the common parts. We should consider using a strategy-like design pattern such that we define a function that takes both the source code and a callback function like:

def _process_lines(code, callback):
    error_collector = ErrorCollector(self.assert_)
    lines = code.split('\n')
    cpp_style.remove_multi_line_comments(lines, error_collector)
    lines = cpp_style.CleansedLines(lines)
    callback(lines, error_collector)
    return error_collector.results()

Then we could write perform_pass_ptr_check() as:

def perform_pass_ptr_check(self, code):
    function_state = cpp_style._FunctionState(self.min_confidence)
    def callback(lines, error_collector):
        for i in xrange(lines.num_lines()):
            cpp_style.detect_functions(lines, i, function_state, error_collector)
            cpp_style.check_pass_ptr_usage(lines, i, function_state, error_collector)
    return _process_lines(code, callback)

Similarly, we could clean up CppStyleTestBase.perform_single_line_lint(), CppStyleTestBase.perform_multi_line_lint(), CppStyleTestBase.perform_language_rules_check(), CppStyleTestBase.perform_function_lengths_check(), and CppStyleTestBase.perform_include_what_you_use().

It may be best to do this cleanup/refactoring in a separate bug.
Comment 3 Daniel Bates 2010-11-14 15:47:54 PST
(In reply to comment #2)
> (From update of attachment 73856 [details])
> [...]
> def _process_lines(code, callback):
>     error_collector = ErrorCollector(self.assert_)
>     lines = code.split('\n')
>     cpp_style.remove_multi_line_comments(lines, error_collector)
>     lines = cpp_style.CleansedLines(lines)
>     callback(lines, error_collector)
>     return error_collector.results()
> 

I meant to write the function prototype as: def _process_lines(self, code, callback). Even better if we could make this a class method.
Comment 4 Daniel Bates 2010-11-14 16:12:22 PST
Comment on attachment 73856 [details]
Patch

Thank you David for filing bug #49519 to clean up the duplicate code.

This patch looks sane to me.

r=me.
Comment 5 David Levin 2010-11-14 16:16:40 PST
Comment on attachment 73856 [details]
Patch

cq-, Will verbosify the ChangeLog a little :)
Comment 6 David Levin 2010-11-14 16:43:25 PST
Created attachment 73864 [details]
Patch
Comment 7 David Levin 2010-11-14 16:45:50 PST
Only changed the ChangeLog and added the word "For" here:
2748 class PassPtrTest(CppStyleTestBase):
2749     # For http://webkit.org/coding/RefPtr.html
Comment 8 WebKit Commit Bot 2010-11-14 17:16:43 PST
The commit-queue encountered the following flaky tests while processing attachment 73864 [details]:

inspector/elements-panel-xhtml-structure.xhtml

Please file bugs against the tests.  These tests were authored by apavlov@chromium.org and pfeldman@chromium.org.  The commit-queue is continuing to process your patch.
Comment 9 WebKit Commit Bot 2010-11-14 17:17:48 PST
Comment on attachment 73864 [details]
Patch

Clearing flags on attachment: 73864

Committed r71989: <http://trac.webkit.org/changeset/71989>
Comment 10 WebKit Commit Bot 2010-11-14 17:17:54 PST
All reviewed patches have been landed.  Closing bug.