This is an archive of the discontinued LLVM Phabricator instance.

Move from a long list of checkers to tables
ClosedPublic

Authored by sylvestre.ledru on Jul 29 2017, 2:48 PM.

Details

Summary

Currently, the list isn't very useful.
This change adds two tables.

  • The checkers
  • The aliases

For each checkers, we provide extract info:

I used the cvs format for the table because:

  • it is easy
  • the data could be reused by other tools (we could move

that into a separated / generated file at some point)

Diff Detail

Event Timeline

would be a classification of checks in general interesting?

i could think of "quality", some checks might be heuristic, some might be new and have some bugs, and some might catch everything correct and handle it perfectly.

So checks could have 2 categories:

-> autofixing/warning
-> always correct/heuristic/unsure

This would maybe improve acceptance, since you know what you get. If you are not interested in toying around, just activate the "always correct" stuff, and you wont accidentially break your code.

Maybe instead of a separate list, having this information like yes/no column in a table in the original is more user-friendly.

Good ideas guys. I will try to update list.rst to a table with the extra information.

I think will be good idea to do this in -list-checks too.

alexfh edited edge metadata.Aug 3 2017, 8:18 AM

Maybe instead of a separate list, having this information like yes/no column in a table in the original is more user-friendly.

What the better format is would depend on for which purpose do we want to expose this information and how it will likely be used. Does anyone have a clear idea about this?

Another point is that it would be much better if we come up with a reliable way to keep this information up-to-date. I currently see a few ways to find which checks are able to issue fix-its:

  1. Analyze lit tests: each test contains the name of the check and most of them use the check_clang_tidy script and CHECK-FIXES prefix to verify fixes. This only works for checks that have lit tests, but the good thing is that we'll only list checks that have test for fixes ;)
  2. Analyze checks' source code for the presence of FixItHint or utility functions that generate it.
  3. Require checks to issue a specific call (or be registered with a certain flag) to enable fix-its. This way we'll have this information in runtime and will be able to expose it in -list-checks.
  4. Analyze checks' documentation and require it to contain a certain pattern, if the check supports fixes.

But first we need to answer the question on how this information is going to be used.

alexfh requested changes to this revision.Aug 3 2017, 8:19 AM
This revision now requires changes to proceed.Aug 3 2017, 8:19 AM

In my opinion, this list should be available to the executable for two reasons.

  1. External tools, that want to integrate clang-tidy in their workflow should be able to get the information about fixits, since they could collidie with their own textual replacements
  2. One might enable/disable only fixing checks. This one is less important, since the -fix flag is available.

Having it written in code, like maybe a flag you set when subclassing the check, makes it ez to automatically determine if the check is modifying for documentation as well.

sylvestre.ledru edited the summary of this revision. (Show Details)

Move to the table idea... Two years after :)

Herald added a project: Restricted Project. · View Herald Transcript


here is the result


here is the result

Thank you thank you, I just almost submitted a patch for removing the second and onwards headings from the ToC.

But please, let's make that table heading nice. Severity is capitalised, name is not, "has an autofix" sounds weird... Maybe "offers fixes" instead?

May be also split table by checks groups?

Polish the column titles

Remove artifacts

But please, let's make that table heading nice. Severity is capitalised, name is not, "has an autofix" sounds weird... Maybe "offers fixes" instead?

Good point, thanks :)
Done!

May be also split table by checks groups?

Maybe? I would like to see this patch land first and iterate then on it. :)
This patch is already a significant improvement over the current state!

I am a little bit conflicted about the Severity column. While I know our people put a great deal of effort into keeping this classification sane, what was put into CodeChecker is, at the end of the day, a pretty arbitrary classification.

I think RSTs support comments, right? Maybe it should be indicated in the code (as a comment only) where the severity was taken from, so if someone comes up with a reason to change a checker's severity, they could also submit a patch to CodeChecker's knowledge-base.

Maybe not just in the code comment, but also in the markup that gets rendered for the users, too?

Add a comment about codechecker

I am a little bit conflicted about the Severity column. While I know our people put a great deal of effort into keeping this classification sane, what was put into CodeChecker is, at the end of the day, a pretty arbitrary classification.

Sure, static analysis is often subjective... Coverity has some classification too. From my experience, the one from code checker matches pretty well.

I think RSTs support comments, right? Maybe it should be indicated in the code (as a comment only) where the severity was taken from, so if someone comes up with a reason to change a checker's severity, they could also submit a patch to CodeChecker's knowledge-base.

Added, thanks for the idea!

Fix a rst warning + update the clang link "Clang Static Analyzer"

Eugene.Zelenko added inline comments.Dec 20 2019, 9:51 AM
clang-tools-extra/docs/clang-tidy/checks/list.rst
291

Aliases?

alias => aliases

sylvestre.ledru marked an inline comment as done.Dec 20 2019, 1:33 PM
sylvestre.ledru added a reviewer: Eugene.Zelenko.
sylvestre.ledru retitled this revision from [clang-tidy] List the checkers with autofix to Move from a long list of checkers to tables.Dec 20 2019, 3:36 PM
sylvestre.ledru edited the summary of this revision. (Show Details)
sylvestre.ledru edited the summary of this revision. (Show Details)
This revision was not accepted when it landed; it landed in state Needs Review.Dec 23 2019, 9:45 AM
This revision was automatically updated to reflect the committed changes.
malcolm.parsons added inline comments.
clang-tools-extra/docs/clang-tidy/checks/list.rst
205

modernize-make-unique offers fixes.

It is in MakeSmartPtrCheck.{h,cpp}, as unique_ptr and shared_ptr can be treated the same.

This list used to be automatically updated by the clang-tools-extra/clang-tidy/add_new_check.py script. I suppose, this is broken now?