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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
