Since clang-tidy supports use of the static analyzer there
should be documentation of how to invoke the static analyzer
checks.
Details
Details
- Reviewers
JonasToth aaron.ballman NoQ Szelethus - Commits
- rG6722923c3883: [clang-tidy] Adding static analyzer check to list of clang-tidy checks
rCTE367694: [clang-tidy] Adding static analyzer check to list of clang-tidy checks
rL367694: [clang-tidy] Adding static analyzer check to list of clang-tidy checks
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 34652 Build 34651: arc lint + arc unit
Event Timeline
Comment Actions
I'm worried that this will continue to drift out of sync with the static analyzer checks unless we find some way to automate it. I wonder if we could write a script to generate these .rst files and somehow fit it into the workflow for adding new static analyzer checks to re-run the script to produce new files (or modified files) as needed? @NoQ and @Szelethus may have ideas.
Comment Actions
I totally agree. The list of checkers changes fairly often.
The checkers are listed in a machine-readable tablegen file, include/clang/StaticAnalyzer/Checkers/Checkers.td.
Note that some of the checkers in the list don't emit any warnings (which is fine in the Static Analyzer as checkers can interact with each other and the only purpose of these checkers is to make other checkers be more correct). There's a related set of checkers that are "hidden" (marked as Hidden in Checkers.td) - those checkers that we don't support enabling/disabling manually, so you should probably not list them either. I think currently all or most of the non-warning checkers are also hidden.
Comment Actions
Just thinking aloud!
We were tinkering with the idea of a checker creator script similar to what clang-tidy has, that could help on this potentially.
Is generating the rst code with Tblgen a feasable approach? At first glance, it sounds like a nightmare to implement, and I wonder whether the maintainers of sphinx buildbots would need to adjust to that.
Comment Actions
clang-tidy doesn't currently rely on tablegen for producing docs like we do for attributes or command line arguments in Clang itself, but I don't think we need that for an initial offering anyway. I'd be happy with a simple python script that we execute as-needed (like we do for AST matchers, for instance) to generate docs that are committed. I figured that python script could execute a tablegen command to offload some of that work. Eventually, we might get to a point where that script can then be run on a server like we do elsewhere.
| clang-tools-extra/docs/clang-tidy/checks/clang-analyzer-apiModeling.StdCLibraryFunctions.rst | ||
|---|---|---|
| 7 | The naming convention for links on the static analyzer documentation makes it difficult to properly generate hyperlinks. | |
Comment Actions
May be script should generate documentation during build, so files .rst files are not needed?
Please also insert empty line between header (===) and text.
Comment Actions
I'd personally weakly advise against that, to be honest;
that would not be inherently bad though, i think.
Please also insert empty line between header (===) and text.
Is there some docs for these checks on clang analyzer side? (will there be?)
Perhaps these can link to those main docs?
Comment Actions
Docs exist for these checks on the analyzer side, they're just all on the same page and hyperlinking to them individually is difficult because the links have a nonstandard naming convention. Currently I'm just linking to that page.
Comment Actions
I think it'll be reasonable to standardize Static Analyzer documentation links first.
| clang-tools-extra/docs/clang-tidy/checks/gen-static-analyzer-docs.py | ||
|---|---|---|
| 2–3 ↗ | (On Diff #209066) | Add full stops to the end of the sentences. |
| 6 ↗ | (On Diff #209066) | Will this work with the monorepo layout? |
| 12 ↗ | (On Diff #209066) | path -> the path Add a full stop to the end of the comment. (Please check the other comments in the file as well.) |
| 73 ↗ | (On Diff #209066) | Is there a way we can automatically link directly to the proper anchor instead of the general checker page? |
Comment Actions
- Added script for generation of docs based off Checkers.td
- Updated to match newly standardized anchor urls
| clang-tools-extra/docs/clang-tidy/checks/gen-static-analyzer-docs.py | ||
|---|---|---|
| 67 ↗ | (On Diff #209260) | For our other aliased checks, we usually add: .. meta:: :http-equiv=refresh: 5;URL=whatever.html so that the redirect is automatic. I think we should probably retain that here as well, WDYT? |
Comment Actions
- Added script for generation of docs based off Checkers.td
- Updated to match newly standardized anchor urls
- Add auto redirect and remove alpha checkers
Comment Actions
I think this looks reasonable to me, though I am still not certain if the relative path in the python script will work with both the svn in-tree directory layout as well as the git monorepo layout (which I'm far less familiar with).
Comment Actions
I'm fairly certain it wouldn't. If I don't forget it, I'll take a look when I get home ;)
Comment Actions
This works in the git monorepo layout as that's what I'm using. Not sure if it works with the svn in-tree directory layout.
Comment Actions
I use svn in-tree, so I tried it out and it does not seem to work for me.
c:\llvm\tools\clang\tools\extra\docs\clang-tidy\checks>python gen-static-analyzer-docs.py
Traceback (most recent call last):
File "gen-static-analyzer-docs.py", line 120, in <module>
main()
File "gen-static-analyzer-docs.py", line 109, in main
checkers = get_checkers()
File "gen-static-analyzer-docs.py", line 23, in get_checkers
result = subprocess.run(["llvm-tblgen", "--dump-json", "-I",
AttributeError: 'module' object has no attribute 'run'
c:\llvm\tools\clang\tools\extra\docs\clang-tidy\checks>python --version
Python 2.7.16Comment Actions
Via: http://llvmweekly.org/issue/289
Tom Stellard reminds us that LLVM's subversion is scheduled to be retired by Oct 21 2019, and enourages everyone to move their workflow to GitHub as soon as possible. See the GitHub migration status for more info.
@aaron.ballman should we really be spending time supporting a workflow that is deprecated and due for removal?
Comment Actions
Yes -- svn is still an official way to work with our repositories and it must continue to work until we retire it.
| clang-tools-extra/docs/clang-tidy/checks/gen-static-analyzer-docs.py | ||
|---|---|---|
| 113 ↗ | (On Diff #211958) | I'm not super enthusiastic about this approach because the default is known to be wrong for supported ways of building Clang. It does give us some way forward, but getting the default wrong for some number of our developers is unfortunate. Why not test the path you list there (which should be good for monorepo) and if that isn't found, test ../../../../../include/clang/StaticAnalyzer/Checkers/ (which should be good for in-tree builds)? This way the arguments only need to be supplied if you have a truly odd directory layout that we can't otherwise guess at. |
Comment Actions
When I run the script locally, I still get issues with subprocess.run().
c:\llvm\tools\clang\tools\extra\docs\clang-tidy\checks>python gen-static-analyzer-docs.py
Traceback (most recent call last):
File "gen-static-analyzer-docs.py", line 148, in <module>
main()
File "gen-static-analyzer-docs.py", line 137, in main
checkers = get_checkers(file_path)
File "gen-static-analyzer-docs.py", line 22, in get_checkers
result = subprocess.run(["llvm-tblgen", "--dump-json", "-I",
AttributeError: 'module' object has no attribute 'run'| clang-tools-extra/docs/clang-tidy/checks/gen-static-analyzer-docs.py | ||
|---|---|---|
| 120–123 ↗ | (On Diff #211967) | Swap the order of these and use an elif so we don't set the path and then overwrite it? |
Comment Actions
This seems reasonable to me except for the fact that the script doesn't run when I try it locally.
c:\llvm\tools\clang\tools\extra\docs\clang-tidy\checks>python gen-static-analyzer-docs.py
Traceback (most recent call last):
File "gen-static-analyzer-docs.py", line 148, in <module>
main()
File "gen-static-analyzer-docs.py", line 137, in main
checkers = get_checkers(file_path)
File "gen-static-analyzer-docs.py", line 22, in get_checkers
result = subprocess.run(["llvm-tblgen", "--dump-json", "-I",
AttributeError: 'module' object has no attribute 'run'
c:\llvm\tools\clang\tools\extra\docs\clang-tidy\checks>python --version
Python 2.7.16I don't think that API is available in Python 2.7, which is our current min required version of Python. That's the last outstanding item for the review that I can see.
Comment Actions
It seems like the list got pretty outdated by the time.
Do you think we should really refer to the clang-analyzer checks in this list? What about simply referring to the clangsa docs instead?
I fear, this will be a constant problem in the future.
That being said, a specific grep-like test would suffice to ensure it's always updated, but I would be surprised as a clang static analyzer developer that I need to run the clang-tools-extra tests besides the clang-analysis ones.
It would cause build bot breakages all the time - and just as many revert commits by choosing that direction.
Comment Actions
Yeah, I was worried this would get out of sycn and it really did not take long for that to happen in practice. I think there's utility in listing the checks in clang-tidy's documentation instead of pointing users to the SA documentation page. Users should have one website to go to where they can see what clang-tidy checks are available to them.
That being said, a specific grep-like test would suffice to ensure it's always updated, but I would be surprised as a clang static analyzer developer that I need to run the clang-tools-extra tests besides the clang-analysis ones.
It would cause build bot breakages all the time - and just as many revert commits by choosing that direction.
So long as pre-commit CI runs the test, I think this might be a reasonable path forward. You may forget to run the tests locally, but you should still get some failure indication before we land a patch. That should also curtail the churn from having to revert. WDYT?
Comment Actions
I was a bit confused as well about the ClangSA support in clang-tidy. What's the current status? Is clang-tidy running *all* checks from StaticAnalyzer?
Comment Actions
If your package is built with CSA enabled, and you don't want to run Cross Translation Unit analysis mode, then yeah. After all, both Clang-Tidy and Clang Static Analyser are just "FrontendAction" libraries under the hood.
Does this still apply given that the quality and formatting match of the documentation website increased over the past one or two releases? The old website if I see correctly no longer lists the checkers (and the previous format was also increasingly outdated and bad experience...), and we have the RST-based new website.
Come to think of it, maybe some RST hacks could automate "pulling in" the list from the (new) CSA website to the Tidy list, as long as maybe they are in a separate category (heading) in the Tidy list?
Although I'm unsure how much being /tools/clang/ and /tools/clang/tools/extra/ mess this thing up.
In case of Python, for example, there is a well-understood way of registering the documentation site of third-party packages, and your documentation generator can automatically generate the cross-reference that links to the other, from your doc's point-of-view, external website. But that involves a considerable amount of "magic".
As far as I know, Sphinx is written in Python, so one could write Python code that runs during the documentation generation (at least the RST part, I'm not sure about the man or infopages or groff stuff!) that can do "magic".
What we COULD do, however, is get rid of the individual checker documentation files (which all just contain machine generated redirects, at least according to this diff...) and have ALL the cross-referencing links (hand-written, autogenerated by our integration, or autogenerated by RST magic) point to not a page living inside Tidy's scope that just does a redirect, but to the appropriate heading in the (new) CSA doc suite?
(Then again, irrespective of what magic we will use, the non-trivial grouping structure on the new CSA docs site can become a problem...)
Comment Actions
I think that clang-tidy users should be able to view the clang-tidy docs and see what checks are available to them (including static analyzer checks). Similarly, I think CSA users should be able to view the CSA docs and see what checks are available to them (including the clang-tidy checks that I think we're now exposing or were exploring exposing).
Come to think of it, maybe some RST hacks could automate "pulling in" the list from the (new) CSA website to the Tidy list, as long as maybe they are in a separate category (heading) in the Tidy list?
Although I'm unsure how much being /tools/clang/ and /tools/clang/tools/extra/ mess this thing up.
In case of Python, for example, there is a well-understood way of registering the documentation site of third-party packages, and your documentation generator can automatically generate the cross-reference that links to the other, from your doc's point-of-view, external website. But that involves a considerable amount of "magic".
As far as I know, Sphinx is written in Python, so one could write Python code that runs during the documentation generation (at least the RST part, I'm not sure about the man or infopages or groff stuff!) that can do "magic".
That's a neat idea! So long as it doesn't become a maintenance burden for some reason, I think it could possibly be a viable path forward.
What we COULD do, however, is get rid of the individual checker documentation files (which all just contain machine generated redirects, at least according to this diff...) and have ALL the cross-referencing links (hand-written, autogenerated by our integration, or autogenerated by RST magic) point to not a page living inside Tidy's scope that just does a redirect, but to the appropriate heading in the (new) CSA doc suite?
(Then again, irrespective of what magic we will use, the non-trivial grouping structure on the new CSA docs site can become a problem...)
If I understand correctly, that would then get rid of the concrete list of checks from list.rst, wouldn't it? Otherwise, we're still going to get out of date trivially as new CSA checks are added but people forget to update list.rst. (Or did I misunderstand?)
Please adjust length. Same in other files.