Turn off lint pre-merge checks for test/

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Turn off lint pre-merge checks for test/

David Chisnall via cfe-dev
Would it be possible to turn off the lint pre-merge checks for
the test/ folder? The suggestion to run clang-format on the tests
is not useful and adds a lot of noise to the diff.

Bruno Ricci
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Turn off lint pre-merge checks for test/

David Chisnall via cfe-dev
On Mon, Jul 6, 2020 at 8:39 AM Bruno Ricci via cfe-dev
<[hidden email]> wrote:
>
> Would it be possible to turn off the lint pre-merge checks for
> the test/ folder? The suggestion to run clang-format on the tests
> is not useful and adds a lot of noise to the diff.

Strong +1. Same for running clang-tidy checks, etc.

~Aaron

>
> Bruno Ricci
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Turn off lint pre-merge checks for test/

David Chisnall via cfe-dev
Maybe it is time to add capability for a .clang-format-ignore file that can work across the tree, then those that want to turn them off could? 

There are some areas where the tests are already clean. It would be a shame to not keep them so, but I understand that FileCheck sometimes might need comments with length that don't meet the style guide

For now could you simply add a local .clang_format file in the test directory to turn clang-format off

e.g.
--------------------------------------------------
BasedOnStyle:LLVM
DisableFormat: true
--------------------------------------------------  

MyDeveloperDay

On Mon, Jul 6, 2020 at 1:47 PM Aaron Ballman via cfe-dev <[hidden email]> wrote:
On Mon, Jul 6, 2020 at 8:39 AM Bruno Ricci via cfe-dev
<[hidden email]> wrote:
>
> Would it be possible to turn off the lint pre-merge checks for
> the test/ folder? The suggestion to run clang-format on the tests
> is not useful and adds a lot of noise to the diff.

Strong +1. Same for running clang-tidy checks, etc.

~Aaron

>
> Bruno Ricci
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Turn off lint pre-merge checks for test/

David Chisnall via cfe-dev
Some of the test folders already have .clang-format files which just
override the column limit
----------------------------------------------------
BasedOnStyle: LLVM
ColumnLimit: 0
----------------------------------------------------
If the issues with formatting are just FileCheck comments being too
long, this is a probably the way to go.
If the issue with formatting is say trying to extract some kind of
diagnostic it may be wise to just wrap the bad code in `clang-format
on/off` comments.
Losing all formatting of test files would be a step backwards WDYT?

~Nathan

On Mon, 2020-07-06 at 13:55 +0100, MyDeveloper Day via cfe-dev wrote:

> Maybe it is time to add capability for a .clang-format-ignore file
> that can work across the tree, then those that want to turn them off
> could?
>
> There are some areas where the tests are already clean. It would be a
> shame to not keep them so, but I understand that FileCheck sometimes
> might need comments with length that don't meet the style guide
>
> For now could you simply add a local .clang_format file in the test
> directory to turn clang-format off
>
> e.g.
> --------------------------------------------------
> BasedOnStyle:LLVM
> DisableFormat: true
> --------------------------------------------------  
>
> MyDeveloperDay
>
> On Mon, Jul 6, 2020 at 1:47 PM Aaron Ballman via cfe-dev <
> [hidden email]> wrote:
> > On Mon, Jul 6, 2020 at 8:39 AM Bruno Ricci via cfe-dev
> > <[hidden email]> wrote:
> > >
> > > Would it be possible to turn off the lint pre-merge checks for
> > > the test/ folder? The suggestion to run clang-format on the tests
> > > is not useful and adds a lot of noise to the diff.
> >
> > Strong +1. Same for running clang-tidy checks, etc.
> >
> > ~Aaron
> >
> > >
> > > Bruno Ricci
> > > _______________________________________________
> > > cfe-dev mailing list
> > > [hidden email]
> > > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
> > _______________________________________________
> > cfe-dev mailing list
> > [hidden email]
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Turn off lint pre-merge checks for test/

David Chisnall via cfe-dev
In reply to this post by David Chisnall via cfe-dev


On 06/07/2020 13:55, MyDeveloper Day wrote:
> Maybe it is time to add capability for a .clang-format-ignore file that can work across the tree, then those that want to turn them off could? 
>
> There are some areas where the tests are already clean. It would be a shame to not keep them so, but I understand that FileCheck sometimes might need comments with length that don't meet the style guide
>
> For now could you simply add a local .clang_format file in the test directory to turn clang-format off

I'm not sure I follow.. .What do you mean by "local"? Local to my tree or local to the test/ folder?
I was talking about the warnings which comes up in the Phab interface.

Bruno

>
> e.g.
> --------------------------------------------------
> BasedOnStyle:LLVM
> DisableFormat: true
> --------------------------------------------------  
>
> MyDeveloperDay
>
> On Mon, Jul 6, 2020 at 1:47 PM Aaron Ballman via cfe-dev <[hidden email] <mailto:[hidden email]>> wrote:
>
>     On Mon, Jul 6, 2020 at 8:39 AM Bruno Ricci via cfe-dev
>     <[hidden email] <mailto:[hidden email]>> wrote:
>     >
>     > Would it be possible to turn off the lint pre-merge checks for
>     > the test/ folder? The suggestion to run clang-format on the tests
>     > is not useful and adds a lot of noise to the diff.
>
>     Strong +1. Same for running clang-tidy checks, etc.
>
>     ~Aaron
>
>     >
>     > Bruno Ricci
>     > _______________________________________________
>     > cfe-dev mailing list
>     > [hidden email] <mailto:[hidden email]>
>     > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>     _______________________________________________
>     cfe-dev mailing list
>     [hidden email] <mailto:[hidden email]>
>     https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Turn off lint pre-merge checks for test/

David Chisnall via cfe-dev
In reply to this post by David Chisnall via cfe-dev


On 06/07/2020 14:04, Nathan James via cfe-dev wrote:

> Some of the test folders already have .clang-format files which just
> override the column limit
> ----------------------------------------------------
> BasedOnStyle: LLVM
> ColumnLimit: 0
> ----------------------------------------------------
> If the issues with formatting are just FileCheck comments being too
> long, this is a probably the way to go.
> If the issue with formatting is say trying to extract some kind of
> diagnostic it may be wise to just wrap the bad code in `clang-format
> on/off` comments.
> Losing all formatting of test files would be a step backwards WDYT?
>
> ~Nathan
>

Hum, so there is already test/.clang-format which does the above.

In the patch which motivated this email (https://reviews.llvm.org/D83183)
most of the noise is caused by complaints about the indentation of CHECK lines.

Perhaps I just need to do a quick clang-format and forget about it.

Bruno
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Turn off lint pre-merge checks for test/

David Chisnall via cfe-dev
In that specific example it looks like clang-format is actually being
helpful for the test file. If clang-format findings start to break test
cases thats when something should be done to silence them.

~Nathan
On Mon, 2020-07-06 at 14:31 +0100, Bruno Ricci via cfe-dev wrote:

>
> On 06/07/2020 14:04, Nathan James via cfe-dev wrote:
> > Some of the test folders already have .clang-format files which
> > just
> > override the column limit
> > ----------------------------------------------------
> > BasedOnStyle: LLVM
> > ColumnLimit: 0
> > ----------------------------------------------------
> > If the issues with formatting are just FileCheck comments being too
> > long, this is a probably the way to go.
> > If the issue with formatting is say trying to extract some kind of
> > diagnostic it may be wise to just wrap the bad code in `clang-
> > format
> > on/off` comments.
> > Losing all formatting of test files would be a step backwards WDYT?
> >
> > ~Nathan
> >
>
> Hum, so there is already test/.clang-format which does the above.
>
> In the patch which motivated this email (
> https://reviews.llvm.org/D83183)
> most of the noise is caused by complaints about the indentation of
> CHECK lines.
>
> Perhaps I just need to do a quick clang-format and forget about it.
>
> Bruno
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Turn off lint pre-merge checks for test/

David Chisnall via cfe-dev
On Mon, Jul 6, 2020 at 9:49 AM Nathan James via cfe-dev
<[hidden email]> wrote:
>
> In that specific example it looks like clang-format is actually being
> helpful for the test file. If clang-format findings start to break test
> cases thats when something should be done to silence them.

It's suggesting formatting changes that aren't typically applied in
FileCheck tests because FileCheck is usually whitespace sensitive for
them.

~Aaron

>
> ~Nathan
> On Mon, 2020-07-06 at 14:31 +0100, Bruno Ricci via cfe-dev wrote:
> >
> > On 06/07/2020 14:04, Nathan James via cfe-dev wrote:
> > > Some of the test folders already have .clang-format files which
> > > just
> > > override the column limit
> > > ----------------------------------------------------
> > > BasedOnStyle: LLVM
> > > ColumnLimit: 0
> > > ----------------------------------------------------
> > > If the issues with formatting are just FileCheck comments being too
> > > long, this is a probably the way to go.
> > > If the issue with formatting is say trying to extract some kind of
> > > diagnostic it may be wise to just wrap the bad code in `clang-
> > > format
> > > on/off` comments.
> > > Losing all formatting of test files would be a step backwards WDYT?
> > >
> > > ~Nathan
> > >
> >
> > Hum, so there is already test/.clang-format which does the above.
> >
> > In the patch which motivated this email (
> > https://reviews.llvm.org/D83183)
> > most of the noise is caused by complaints about the indentation of
> > CHECK lines.
> >
> > Perhaps I just need to do a quick clang-format and forget about it.
> >
> > Bruno
> > _______________________________________________
> > cfe-dev mailing list
> > [hidden email]
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev