aaron.ballman marked 4 inline comments as done.
aaron.ballman added inline comments.
================
Comment at: lib/StaticAnalyzer/Core/SarifDiagnostics.cpp:129
+ unsigned Index = 0;
+ for (const json::Value &File : Files) {
+ if (const json::Object *Obj = File.getAsObject()) {
----------------
NoQ wrote:
> This sounds like `find_if` to me.
The problem is: we need the `Index`. It felt a bit weird to have
`llvm::find_if()` (a const operation) also mutating a locally-captured
variable. WDYT?
================
Comment at: lib/StaticAnalyzer/Core/SarifDiagnostics.cpp:143
+ // that an empty file lists always causes a file to be added.
+ if (Files.empty() || Index == Files.size())
+ Files.push_back(createFile(FE));
----------------
NoQ wrote:
> I suspect that the LHS of `||` implies the RHS of `||` and is therefore
> redundant.
Nope, this is needed for a boundary condition. In the case where `Files` is
empty, we don't loop over anything in the range-based for loop, and so `Index`
is zero, which means `Index == Files.size()`. On the other end of that
boundary, if `Files` is nonempty but `FE` hasn't been added to it yet, you'll
loop over the entire list of `Files` in the range-based for loop, which also
triggers `Index == Files.size()`.
================
Comment at: lib/StaticAnalyzer/Core/SarifDiagnostics.cpp:289
+ if (P.second) {
+ RuleMapping[RuleID] = Rules.size(); // Maps RuleID to an Array Index.
+ Rules.push_back(createRule(*D));
----------------
NoQ wrote:
> Aha, ok, so now instead of an alphabetical order we have the order in which
> reports of the respective checkers are squeezed into the consumer. Which is
> probably deterministic, so it's fine. I just enjoy talking to myself on
> phabricator sometimes.
Yes, this should be deterministic -- good thought to check!
================
Comment at: lib/StaticAnalyzer/Core/SarifDiagnostics.cpp:318
void SarifDiagnostics::FlushDiagnosticsImpl(
std::vector<const PathDiagnostic *> &Diags, FilesMade *) {
// We currently overwrite the file if it already exists. However, it may be
----------------
NoQ wrote:
> I wonder why we didn't mark `Diags` as `const &` in this callback.
It would be a nice refactoring for someday. :-)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55707/new/
https://reviews.llvm.org/D55707
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
