Bug 134620 – Teach check-webkit-style to suggest WTF::move() when it sees std::move()

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

[Daniel Bates]

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.

[Daniel Bates]

Created attachment 234383 [details]
Patch and unit test

[Joseph Pecoraro]

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

[Daniel Bates]

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

[Daniel Bates]

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

[Daniel Bates]

Committed r171065: <http://trac.webkit.org/changeset/171065>