Bug 134620 - Teach check-webkit-style to suggest WTF::move() when it sees std::move()
Summary: Teach check-webkit-style to suggest WTF::move() when it sees std::move()
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: Daniel Bates
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-07-03 17:25 PDT by Daniel Bates
Modified: 2014-07-14 09:17 PDT (History)
11 users (show)

See Also:


Attachments
Patch and unit test (4.69 KB, patch)
2014-07-03 17:32 PDT, Daniel Bates
joepeck: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2014-07-03 17:25:59 PDT
Following the patch for bug #134500, we should teach check-webkit-style to suggest using WTF::move() instead of std::move(). The former is less error prone to use than the latter. See bug #134500 for more details.
Comment 1 Daniel Bates 2014-07-03 17:32:26 PDT
Created attachment 234383 [details]
Patch and unit test
Comment 2 Joseph Pecoraro 2014-07-03 17:44:35 PDT
Comment on attachment 234383 [details]
Patch and unit test

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

Nice! r=me

> Tools/Scripts/webkitpy/style/checkers/cpp.py:2286
> +    # This check doesn't apply to C or Objective-C implementation files.
> +    if file_state.is_c_or_objective_c():
> +        return

We want this for ObjC++. This "is_c_or_objective_c" to too generic for that. A quick search found:

  network/ios/QuickLook.mm
  431:    return WTF::move(quickLookHandle);
  445:    return WTF::move(quickLookHandle);
  463:    return WTF::move(quickLookHandle);

Maybe we would want an "is_cpp".
Comment 3 Daniel Bates 2014-07-14 07:38:09 PDT
(In reply to comment #2)
> (From update of attachment 234383 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234383&action=review
> 
> Nice! r=me
> 
> > Tools/Scripts/webkitpy/style/checkers/cpp.py:2286
> > +    # This check doesn't apply to C or Objective-C implementation files.
> > +    if file_state.is_c_or_objective_c():
> > +        return
> 
> We want this for ObjC++. This "is_c_or_objective_c" to too generic for that. A quick search found:
> 
>   network/ios/QuickLook.mm
>   431:    return WTF::move(quickLookHandle);
>   445:    return WTF::move(quickLookHandle);
>   463:    return WTF::move(quickLookHandle);
> 
> Maybe we would want an "is_cpp".

It seems sufficient to be able to identify a file as Objective-C/Objective-C++ from a C file instead of adding an is_cpp method (see remark (*)). Then we can add a new method, say _FileState.is_objective_c_or_objective_cpp, that returns true for files that end in ".m" or ".mm" or header files that contain Objective-C directives and repurpose _FileState.is_c_or_objective_c() to only return true for files that end in ".c" or ".m" or header files that contain 'extern "C"' or Objective-C directives.

(*) From my understanding the C++ style checker module of check-style assumes that its input is a file that contains C++ code and the purpose of the _FileState methods are to detect when the input file may also contain C or Objective-C code with the exception that files that end in ".c" or ".m" are assumed to contain C and Objective-C code, respectively.
Comment 4 Daniel Bates 2014-07-14 09:11:08 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > [...]
> > We want this for ObjC++. This "is_c_or_objective_c" to too generic for that. 
> > [...]
> > Maybe we would want an "is_cpp".
> 
> It seems sufficient to be able to identify a file as Objective-C/Objective-C++ from a C file instead of adding an is_cpp method (see remark (*)). Then we can add a new method, say _FileState.is_objective_c_or_objective_cpp, that returns true for files that end in ".m" or ".mm" or header files that contain Objective-C directives and repurpose _FileState.is_c_or_objective_c() to only return true for files that end in ".c" or ".m" or header files that contain 'extern "C"' or Objective-C directives.
> 
> [...]

I filed bug #134884 to teach check-webkit-style to apply C++ rules to Objective-C++ files if applicable.
Comment 5 Daniel Bates 2014-07-14 09:17:27 PDT
Committed r171065: <http://trac.webkit.org/changeset/171065>