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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Global nit: I guess having both DESC and DOCS as variable/macro arg names is confusing. Can we instead use DOCSURI/DocsUri?

In D55792#1334339 <https://reviews.llvm.org/D55792#1334339>, @aaron.ballman wrote:

> In D55792#1334291 <https://reviews.llvm.org/D55792#1334291>, @Szelethus wrote:
>
> > Thank you so much for working on this! The lack of a 
> >  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?


Well, we already store checker- and the quite lengthy -analyzer-config descriptions... but I'm not sure thats a good point.
However, plugin checkers being able to specify uris such as "mycompany.internal.supersecret.idk/dontshare/checkers/tradesecrets.html" would be neat.

One more thing to consider is that if Sphinx lands after 8.0.0., the uri will most probably change. We can always redirect, but thats a thing to consider. I'm not sure whether there's any likelihood of it being completed until the new release branch is created. But I don't think there's any change needed for this patch just because of this.



================
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
----------------
aaron.ballman wrote:
> 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.
Ah, touchpad messed up my inline, but I guess it was understandable.

Okay, thanks, one more concern out of the way then!


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

https://reviews.llvm.org/D55792