Szelethus added a comment.

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

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`.

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.



================
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
----------------
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?


================
Comment at: lib/StaticAnalyzer/Core/SarifDiagnostics.cpp:284-286
+  return (llvm::Twine("https://clang-analyzer.llvm.org/";) + DocLoc + "#" +
+          CheckName)
+      .str();
----------------
Why not do this in `ClangSACheckersEmitter.cpp`?


================
Comment at: utils/TableGen/ClangSACheckersEmitter.cpp:90-91
 
+  OS << "// This file is automatically generated. Do not edit this file by "
+        "hand.\n";
+
----------------
Cheers!


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

https://reviews.llvm.org/D55792



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to