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