[PATCH] D55707: Update to SARIF 11-28

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

Yup, looks good, and i keep passively cheering for this project to be successful.



================
Comment at: lib/StaticAnalyzer/Core/SarifDiagnostics.cpp:129
+  unsigned Index = 0;
+  for (const json::Value &File : Files) {
+    if (const json::Object *Obj = File.getAsObject()) {
----------------
This sounds like `find_if` to me.


================
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));
----------------
I suspect that the LHS of `||` implies the RHS of `||` and is therefore redundant.


================
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));
----------------
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.


================
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
----------------
I wonder why we didn't mark `Diags` as `const &` in this callback.


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

https://reviews.llvm.org/D55707