[llvm-dev] merge_guards_bot reports clang-tidy/clang-format findings unrelated to modified code

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[llvm-dev] merge_guards_bot reports clang-tidy/clang-format findings unrelated to modified code

Jeremy Morse via llvm-dev
First, I love this bot:) It makes LLVM's "pushing to master" practise less awful:)
Now the main topic...

> clang-tidy: fail. Please fix clang-tidy findings.

For example, on https://reviews.llvm.org/D72103#1801916 ,
merge_guards_bot reports clang-tidy findings of existing code, not just
the modified code.

I think https://github.com/google/llvm-premerge-checks should use
--line-filter= as clang-tidy/tool/clang-tidy-diff.py does.

> clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Similarly, clang-format should use --lines to filter out untouched code.
_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] merge_guards_bot reports clang-tidy/clang-format findings unrelated to modified code

Jeremy Morse via llvm-dev
I second Fangrui's findings and add:

- clang-format should only run on "source" files. For one, it runs on
  non-C/C++ files right now with the results you can imagine. Even if it
  is a C/C++ test we do not always want to format it.
- it would be perfect if the linter would not report issues that are
  present in the surrounding code, e.g., if the naming scheme is kept
  consistent with the surrounding code it should not report it as
  problematic. For example, since the functions in ADT/SetOperations.h
  are all flagged as problematic which means if you add one in the same
  style it is flagged as well.


On 01/02, Fangrui Song via llvm-dev wrote:

> First, I love this bot:) It makes LLVM's "pushing to master" practise less awful:)
> Now the main topic...
>
> > clang-tidy: fail. Please fix clang-tidy findings.
>
> For example, on https://reviews.llvm.org/D72103#1801916 ,
> merge_guards_bot reports clang-tidy findings of existing code, not just
> the modified code.
>
> I think https://github.com/google/llvm-premerge-checks should use
> --line-filter= as clang-tidy/tool/clang-tidy-diff.py does.
>
> > clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.
>
> Similarly, clang-format should use --lines to filter out untouched code.
> _______________________________________________
> LLVM Developers mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
--

Johannes Doerfert
Researcher

Argonne National Laboratory
Lemont, IL 60439, USA

[hidden email]

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

signature.asc (235 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] merge_guards_bot reports clang-tidy/clang-format findings unrelated to modified code

Jeremy Morse via llvm-dev
On Fri, Jan 03, 2020 at 04:36:25PM +0000, Doerfert, Johannes via llvm-dev wrote:
> I second Fangrui's findings and add:
>
> - clang-format should only run on "source" files. For one, it runs on
>   non-C/C++ files right now with the results you can imagine. Even if it
>   is a C/C++ test we do not always want to format it.

I already file a bug report to request not to format the tests [1].

[1] https://github.com/google/llvm-premerge-checks/issues/89


Kind regards,
Mark de Wever
_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] merge_guards_bot reports clang-tidy/clang-format findings unrelated to modified code

Jeremy Morse via llvm-dev
[hidden email] 

Hi Fangrui,

I'm glad you like the tool!

We got a bunch of issues reported around clang-tidy, so Mikhail disabled it today. Since the thread has a few other points, I created a separate issue [1] for it.

The linter script [2] is already using clang-tidy-diff on the git diff. So it should only look into changed lines... 


Best,
Christian

On Sat, Jan 4, 2020 at 7:59 PM Mark de Wever via llvm-dev <[hidden email]> wrote:
On Fri, Jan 03, 2020 at 04:36:25PM +0000, Doerfert, Johannes via llvm-dev wrote:
> I second Fangrui's findings and add:
>
> - clang-format should only run on "source" files. For one, it runs on
>   non-C/C++ files right now with the results you can imagine. Even if it
>   is a C/C++ test we do not always want to format it.

I already file a bug report to request not to format the tests [1].

[1] https://github.com/google/llvm-premerge-checks/issues/89


Kind regards,
Mark de Wever
_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


--
Best,
Christian

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev