This is a semi-common mistake for contributors so it would be good to detect it automatically.
Created attachment 73856 [details] Patch
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.
(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 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 on attachment 73856 [details] Patch cq-, Will verbosify the ChangeLog a little :)
Created attachment 73864 [details] Patch
Only changed the ChangeLog and added the word "For" here: 2748 class PassPtrTest(CppStyleTestBase): 2749 # For http://webkit.org/coding/RefPtr.html
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 on attachment 73864 [details] Patch Clearing flags on attachment: 73864 Committed r71989: <http://trac.webkit.org/changeset/71989>
All reviewed patches have been landed. Closing bug.