[webkit-dev] Clang tidy

[Maciej Stachowiak]

> On May 8, 2017, at 9:09 PM, Ryosuke Niwa <rniwa at webkit.org> wrote:
> 
> On Wed, May 3, 2017 at 8:31 PM, Maciej Stachowiak <mjs at apple.com <mailto:mjs at apple.com>> wrote:
>  <x-redundant-cluster-toggle://0> <x-redundant-cluster-toggle://0>On May 3, 2017, at 6:31 PM, Olmstead, Don < <x-redundant-cluster-toggle://0>Don.Olmstead at sony.com <mailto:Don.Olmstead at sony.com>> wrote:
>> I took some time today to see how clang-tidy can be run on WebKit code and
>> openedhttps://bugs.webkit.org/show_bug.cgi?id=171632 <openedhttps://bugs.webkit.org/show_bug.cgi?id=171632> with some examples on
>> how to run things. I also attached some output from the modernizer fixes
>> that can be applied.
>> I was thinking of running any code we move from WebCore/platform through
>> clang-tidy during the process of moving it to PAL. Documentation for the
>> checks can be found at
>> http://clang.llvm.org/extra/clang-tidy/checks/list.htmlif <http://clang.llvm.org/extra/clang-tidy/checks/list.htmlif> anyone wants to
>> take a look at what should potentially be run.
>> I think moving code should be done with the bare minimum changes needed to
>> do the move (file paths, etc). Any code cleanup script should be done
>> separately. Any change can break things (though we hope both moving files
>> and running clang-tidy would have no behavioral effect). Therefore it's best
>> not to mix unrelated changes, so if a regression occurs, it's easier to see
>> what caused it.
> 
>> 
>> 
>> On May 3, 2017, at 6:31 PM, Olmstead, Don <Don.Olmstead at sony.com <mailto:Don.Olmstead at sony.com>> wrote:
>> 
>> I took some time today to see how clang-tidy can be run on WebKit code and
>> openedhttps://bugs.webkit.org/show_bug.cgi?id=171632 <openedhttps://bugs.webkit.org/show_bug.cgi?id=171632> with some examples on
>> how to run things. I also attached some output from the modernizer fixes
>> that can be applied.
>> 
>> I was thinking of running any code we move from WebCore/platform through
>> clang-tidy during the process of moving it to PAL. Documentation for the
>> checks can be found at
>> http://clang.llvm.org/extra/clang-tidy/checks/list.htmlif <http://clang.llvm.org/extra/clang-tidy/checks/list.htmlif> anyone wants to
>> take a look at what should potentially be run.
>> 
>> 
>> I think moving code should be done with the bare minimum changes needed to
>> do the move (file paths, etc). Any code cleanup script should be done
>> separately. Any change can break things (though we hope both moving files
>> and running clang-tidy would have no behavioral effect). Therefore it's best
>> not to mix unrelated changes, so if a regression occurs, it's easier to see
>> what caused it.
> 
> I agree with Maciej and Konstantin that these code changes should be
> made separately due to the risk of inadvertently introducing
> regressions.
> 
> In addition, I don't really like code churns like this (as they make
> SVN/Git blame less useful) unless we're fixing violations of WebKit
> code style guideline: https://webkit.org/code-style-guidelines/ <https://webkit.org/code-style-guidelines/>
> 
> Many of the changes made in the aforementioned bug appear to be more
> of nits that aren't explicitly called out in our guideline.

We could add entries to our style guideline for some of these things if we think they are better. Then at least new code can get them.

Not sure all these things are an improvement though. I'm personally not a fan of including <cwhatever> instead of <whatever.h> and we historically haven't done it that way.

 - Maciej


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20170509/989bf3a4/attachment.html>