Bug 49513 – check-webkit-style should detect PassRefPtr usage in functions.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[David Levin]

This is a semi-common mistake for contributors so it would be good to detect it automatically.

[David Levin]

Created attachment 73856 [details]
Patch

[Daniel Bates]

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.

[Daniel Bates]

(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.

[Daniel Bates]

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.

[David Levin]

Comment on attachment 73856 [details]
Patch

cq-, Will verbosify the ChangeLog a little :)

[David Levin]

Created attachment 73864 [details]
Patch

[David Levin]

Only changed the ChangeLog and added the word "For" here:
2748 class PassPtrTest(CppStyleTestBase):
2749     # For http://webkit.org/coding/RefPtr.html

[WebKit Commit Bot]

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 [e-mail] and [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 73864 [details]
Patch

Clearing flags on attachment: 73864

Committed r71989: <http://trac.webkit.org/changeset/71989>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.