[PATCH] D53814: Allow the analyzer to output to a SARIF file

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

Patch context is missing.



================
Comment at: Analysis/diagnostics/sarif-check.py:22
+passes = 0
+with open(testfile) as testfh:
+    lineno = 0
----------------
Wow, this is super neat!
Since you are quite active in LLVM community, would you think it's better to have this tool in `llvm/utils` next to FileCheck? The concept is very general, and I think a lot of people really feel FileCheck limitations.


================
Comment at: Analysis/diagnostics/sarif-diagnostics-taint-test.c:18
+// Test the basics for sanity.
+// CHECK: sarifLog['version'].startswith("2.0.0")
+// CHECK: sarifLog['runs'][0]['tool']['fullName'] == "clang static analyzer"
----------------
Would it make more sense to just use `diff` + json pretty-formatter to write a test?
With this test I can't even quite figure out how the output should look like.


================
Comment at: StaticAnalyzer/Core/SarifDiagnostics.cpp:74
+  // Add the rest of the path components, encoding any reserved characters.
+  std::for_each(std::next(sys::path::begin(Filename)), sys::path::end(Filename),
+                [&Ret](StringRef Component) {
----------------
Nitpicking style, but I don't see why for-each loop, preferably with a range wrapping the iterators would not be more readable.


================
Comment at: StaticAnalyzer/Core/SarifDiagnostics.cpp:182
+  case PathDiagnosticPiece::Kind::Note:
+    // FIXME: What should be reported here?
+    break;
----------------
"Note" are notes which do not have to be attached to a particular path element.


================
Comment at: StaticAnalyzer/Core/SarifDiagnostics.cpp:243
+
+  llvm::for_each(Diags, [&](const PathDiagnostic *D) {
+    Results.push_back(createResult(*D, Files));
----------------
I like closures, but what's wrong with just using a `for` loop here?


================
Comment at: StaticAnalyzer/Core/SarifDiagnostics.cpp:254
+    std::vector<const PathDiagnostic *> &Diags, FilesMade *) {
+  // FIXME: if the file already exists, do we overwrite it with a single run,
+  // or do we append a run into the file if it's a valid SARIF log?
----------------
Usually we overwrite the file and note that on stderr in such cases.


https://reviews.llvm.org/D53814