Re: [webkit-dev] Clang tidy

[Maciej Stachowiak]

> On May 8, 2017, at 9:09 PM, Ryosuke Niwa  wrote:
> 
> On Wed, May 3, 2017 at 8:31 PM, Maciej Stachowiak  > wrote:
>   On May 3, 
> 2017, at 6:31 PM, Olmstead, Don < 
> don.olmst...@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 
>>  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 
>>  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 > > 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 
>>  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 
>>  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/ 
> 
> 
> 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  instead of  and we historically haven't 
done it that way.

 - Maciej


___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Clang tidy

[Ryosuke Niwa]

On Wed, May 3, 2017 at 8:31 PM, Maciej Stachowiak  wrote:
>
>
> On May 3, 2017, at 6:31 PM, Olmstead, Don  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 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 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/

Many of the changes made in the aforementioned bug appear to be more
of nits that aren't explicitly called out in our guideline.

- R. Niwa
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Clang tidy

[Konstantin Tokarev]

04.05.2017, 04:31, "Olmstead, Don" :
> I took some time today to see how clang-tidy can be run on WebKit code and 
> opened https://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 think you should not mix different kinds of modernizations in the single 
patch. This way it would be easier to review the changes, also changes that are 
already used by community will get in fast as no-brainers, while others may 
need discussion.

My 2c.

>
> 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.html if anyone wants to 
> take a look at what should potentially be run.
> ,
>
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev


-- 
Regards,
Konstantin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Clang tidy

[Maciej Stachowiak]

> On May 3, 2017, at 6:31 PM, Olmstead, Don  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 
>  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.html 
> if 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.

Probably there should also be a discussion of whether we want these particular 
changes. People tend to dislike churn, and we'd also need to make sure the 
newer style works on all toolchains that people use, and that stylistically we 
like #include  in place of #include  and using in place 
of typedef. I don't think we have style rules calling for those things, so it 
would be jumping the gun a bit to start applying them. This is an additional 
reason to separate style changes from moving code to PAL (which I believe has 
been discussed and is not controversial).

Regards,
Maciej

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev