[PATCH] D55792: Allow direct navigation to static analysis checker documentation through SARIF exports

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked 3 inline comments as done.
aaron.ballman added a comment.

In D55792#1334291 <https://reviews.llvm.org/D55792#1334291>, @Szelethus wrote:

> Thank you so much for working on this! The lack of a proper documentation is indeed a weak points of the project.


Yeah, the more we can couple the documentation to the check definitions, the more likely we are to see the documentation remain up to date as checks are added (at least, that's been the case with attributes).

> I guess it'd be better to generate the entire URI in the tblgen file, and also allow plugins to register their own through `CheckerRegistry`. Sarif lacking the support for them is one thing, but adding a new field to the `Checker` class in `CheckerBase.td` I think should be accompanied by extending `CheckerRegistry::CheckerInfo` and `CheckerRegistry::getChecker`.

Good point about the checker registry, I'll make those changes. However, I'm a bit less certain about generating the entire URI in the tablegen file -- that adds a lot of string literals to the binary when I think we don't have to. However, perhaps that's not a huge concern and having the information in its final form in the .inc file is a better approach. WDYT?

> While I dislike the use of `<a id="some.Checker"></a>`, until Sphinx documentation becomes a thing, I guess we can't do much about it.
> 
> But the overall direction of this patch is great.

Thanks, I'm glad to hear it!



================
Comment at: lib/StaticAnalyzer/Core/SarifDiagnostics.cpp:270-275
+#define GET_CHECKERS
+#define CHECKER(FULLNAME, CLASS, HELPTEXT, DOCS)                               \
+  .Case(FULLNAME, DOCS)
+#include "clang/StaticAnalyzer/Checkers/Checkers.inc"
+#undef CHECKER
+#undef GET_CHECKERS
----------------
Szelethus wrote:
> As per usual, this won't handle plugin checkers, but the solution is in an arms reach -- in the last couple months, I changed a lot around checker registration, and once things settle down a bit.
> 
> One idea is to store the list of registered checkers (now, in this case, registered means that the analyzer even knows about its existence, not that it's enabled) in `AnalyzerOptions`, however it'd force you to put an extra parameter to almost all functions here. What do you think?
> As per usual, this won't handle plugin checkers, but the solution is in an arms reach -- in the last couple months, I changed a lot around checker registration, and once things settle down a bit.

I knew this wouldn't do good things for plugins yet, but I'm glad to hear things are moving forward on that front!

> One idea is to store the list of registered checkers (now, in this case, registered means that the analyzer even knows about its existence, not that it's enabled) in AnalyzerOptions, however it'd force you to put an extra parameter to almost all functions here. What do you think?

I could always wrap these methods up to be private methods of the `SarifDiagnostics` class so they'd have access to the options, so I think there's a decent path forward.


================
Comment at: lib/StaticAnalyzer/Core/SarifDiagnostics.cpp:284-286
+  return (llvm::Twine("https://clang-analyzer.llvm.org/") + DocLoc + "#" +
+          CheckName)
+      .str();
----------------
Szelethus wrote:
> Why not do this in `ClangSACheckersEmitter.cpp`?
In part so there will be less string literals, basically. By supplying only the .html target in the .inc file, we have one of three string literals there. I could move the http:// part in to the .inc file as well (we'd still only have the same three string literals), but if I moved the anchor targets as well, we now have N string literals baked into the binary when we don't need to use that much space.


================
Comment at: utils/TableGen/ClangSACheckersEmitter.cpp:90-91
 
+  OS << "// This file is automatically generated. Do not edit this file by "
+        "hand.\n";
+
----------------
Szelethus wrote:
> Cheers!
You make that mistake *one time* and then you add comments to the top of the auto-generated file. ;-)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55792/new/

https://reviews.llvm.org/D55792