Removing the naming checks from clang's .clang-tidy files

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

Removing the naming checks from clang's .clang-tidy files

David Blaikie via cfe-dev
Hi cfe-dev,

Clangd started showing clang-tidy warnings recently and I've noticed there is too much of by clang-tidy inside the clang codebase, and most of it is coming from 'readability-identifier-naming' check (at least in the Sema and Parser code).

E.g. running 
./bin/clang-tidy  ../llvm/clang/lib/Parse/ParseExpr.cpp

produces produces 52 warnings, 51 of which are naming violations from 'readability-identifier-naming'. 'Sema.h' has 1830 clang-tidy warnings with 'readability-identifier-naming' and 228 without it.

IIUC, the consensus is that renaming everything to align with the style guide is just not worth it (would introduce merge conflicts, mess up the history, etc). Does this render the naming check non-useful for the 'clang/' project? Should we remove it from  'clang/.clang-tidy'?

Are there other alternatives that could bring down the noise in clang-tidy output and actually make it useful (e.g. we could put a file-wide NOLINT comments into those files)?

--
Regards,
Ilya Biryukov

_______________________________________________
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: Removing the naming checks from clang's .clang-tidy files

David Blaikie via cfe-dev
We disabled the two most noisy checks in r352862 and clang-tidy now produces only 3 warnings on Sema.h.
Let us know if you have concerns and feel we should revert this.

On Tue, Jan 29, 2019 at 6:03 PM Ilya Biryukov <[hidden email]> wrote:
Hi cfe-dev,

Clangd started showing clang-tidy warnings recently and I've noticed there is too much of by clang-tidy inside the clang codebase, and most of it is coming from 'readability-identifier-naming' check (at least in the Sema and Parser code).

E.g. running 
./bin/clang-tidy  ../llvm/clang/lib/Parse/ParseExpr.cpp

produces produces 52 warnings, 51 of which are naming violations from 'readability-identifier-naming'. 'Sema.h' has 1830 clang-tidy warnings with 'readability-identifier-naming' and 228 without it.

IIUC, the consensus is that renaming everything to align with the style guide is just not worth it (would introduce merge conflicts, mess up the history, etc). Does this render the naming check non-useful for the 'clang/' project? Should we remove it from  'clang/.clang-tidy'?

Are there other alternatives that could bring down the noise in clang-tidy output and actually make it useful (e.g. we could put a file-wide NOLINT comments into those files)?

--
Regards,
Ilya Biryukov


--
Regards,
Ilya Biryukov

_______________________________________________
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: Removing the naming checks from clang's .clang-tidy files

David Blaikie via cfe-dev

I think there could be two modes in which clang-tidy is being run.

For new code seeing these violations would be great, but they could be run as linter in `arc`, for existing code this is of course
another thing.

In my opinion we should lint new code better and have clang-tidy run there at least with full configuration enabled.

Am 01.02.19 um 12:21 schrieb Ilya Biryukov via cfe-dev:
We disabled the two most noisy checks in r352862 and clang-tidy now produces only 3 warnings on Sema.h.
Let us know if you have concerns and feel we should revert this.

On Tue, Jan 29, 2019 at 6:03 PM Ilya Biryukov <[hidden email]> wrote:
Hi cfe-dev,

Clangd started showing clang-tidy warnings recently and I've noticed there is too much of by clang-tidy inside the clang codebase, and most of it is coming from 'readability-identifier-naming' check (at least in the Sema and Parser code).

E.g. running 
./bin/clang-tidy  ../llvm/clang/lib/Parse/ParseExpr.cpp

produces produces 52 warnings, 51 of which are naming violations from 'readability-identifier-naming'. 'Sema.h' has 1830 clang-tidy warnings with 'readability-identifier-naming' and 228 without it.

IIUC, the consensus is that renaming everything to align with the style guide is just not worth it (would introduce merge conflicts, mess up the history, etc). Does this render the naming check non-useful for the 'clang/' project? Should we remove it from  'clang/.clang-tidy'?

Are there other alternatives that could bring down the noise in clang-tidy output and actually make it useful (e.g. we could put a file-wide NOLINT comments into those files)?

--
Regards,
Ilya Biryukov


--
Regards,
Ilya Biryukov

_______________________________________________
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: Removing the naming checks from clang's .clang-tidy files

David Blaikie via cfe-dev
Totally agree, if we had a nice UI (in Phabricator?) or a script to show only clang-tidy warnings touching the change diffs, disabling these checks would have been a much harder sell.
Even in that case, though, we'd probably want to have different configs for clang-tidy-over-diffs and clang-tidy to avoid cluttering the output when using clangd or standalone clang-tidy.

On Fri, Feb 1, 2019 at 2:44 PM Jonas Toth <[hidden email]> wrote:

I think there could be two modes in which clang-tidy is being run.

For new code seeing these violations would be great, but they could be run as linter in `arc`, for existing code this is of course
another thing.

In my opinion we should lint new code better and have clang-tidy run there at least with full configuration enabled.

Am 01.02.19 um 12:21 schrieb Ilya Biryukov via cfe-dev:
We disabled the two most noisy checks in r352862 and clang-tidy now produces only 3 warnings on Sema.h.
Let us know if you have concerns and feel we should revert this.

On Tue, Jan 29, 2019 at 6:03 PM Ilya Biryukov <[hidden email]> wrote:
Hi cfe-dev,

Clangd started showing clang-tidy warnings recently and I've noticed there is too much of by clang-tidy inside the clang codebase, and most of it is coming from 'readability-identifier-naming' check (at least in the Sema and Parser code).

E.g. running 
./bin/clang-tidy  ../llvm/clang/lib/Parse/ParseExpr.cpp

produces produces 52 warnings, 51 of which are naming violations from 'readability-identifier-naming'. 'Sema.h' has 1830 clang-tidy warnings with 'readability-identifier-naming' and 228 without it.

IIUC, the consensus is that renaming everything to align with the style guide is just not worth it (would introduce merge conflicts, mess up the history, etc). Does this render the naming check non-useful for the 'clang/' project? Should we remove it from  'clang/.clang-tidy'?

Are there other alternatives that could bring down the noise in clang-tidy output and actually make it useful (e.g. we could put a file-wide NOLINT comments into those files)?

--
Regards,
Ilya Biryukov


--
Regards,
Ilya Biryukov

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


--
Regards,
Ilya Biryukov

_______________________________________________
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: Removing the naming checks from clang's .clang-tidy files

David Blaikie via cfe-dev

Yes, allowing different configuration files in clang-tidy would be an easy step forward (clang-tidy requires a '.clang-tidy' file and provides no option to specify something else).

See this revision https://reviews.llvm.org/D55523 for some thoughts we have in the clang-tidy space on how to proceed and integrate with phabricator.

Am 01.02.19 um 14:00 schrieb Ilya Biryukov:
Totally agree, if we had a nice UI (in Phabricator?) or a script to show only clang-tidy warnings touching the change diffs, disabling these checks would have been a much harder sell.
Even in that case, though, we'd probably want to have different configs for clang-tidy-over-diffs and clang-tidy to avoid cluttering the output when using clangd or standalone clang-tidy.

On Fri, Feb 1, 2019 at 2:44 PM Jonas Toth <[hidden email]> wrote:

I think there could be two modes in which clang-tidy is being run.

For new code seeing these violations would be great, but they could be run as linter in `arc`, for existing code this is of course
another thing.

In my opinion we should lint new code better and have clang-tidy run there at least with full configuration enabled.

Am 01.02.19 um 12:21 schrieb Ilya Biryukov via cfe-dev:
We disabled the two most noisy checks in r352862 and clang-tidy now produces only 3 warnings on Sema.h.
Let us know if you have concerns and feel we should revert this.

On Tue, Jan 29, 2019 at 6:03 PM Ilya Biryukov <[hidden email]> wrote:
Hi cfe-dev,

Clangd started showing clang-tidy warnings recently and I've noticed there is too much of by clang-tidy inside the clang codebase, and most of it is coming from 'readability-identifier-naming' check (at least in the Sema and Parser code).

E.g. running 
./bin/clang-tidy  ../llvm/clang/lib/Parse/ParseExpr.cpp

produces produces 52 warnings, 51 of which are naming violations from 'readability-identifier-naming'. 'Sema.h' has 1830 clang-tidy warnings with 'readability-identifier-naming' and 228 without it.

IIUC, the consensus is that renaming everything to align with the style guide is just not worth it (would introduce merge conflicts, mess up the history, etc). Does this render the naming check non-useful for the 'clang/' project? Should we remove it from  'clang/.clang-tidy'?

Are there other alternatives that could bring down the noise in clang-tidy output and actually make it useful (e.g. we could put a file-wide NOLINT comments into those files)?

--
Regards,
Ilya Biryukov


--
Regards,
Ilya Biryukov

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


--
Regards,
Ilya Biryukov

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