[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Aside from some minor nits and the open question about validation, I think this is getting pretty close. I'm curious about the next steps with this though, given that there are no in-tree uses for it currently. You had mentioned you planned to work on an adapter so that we could eventually start to remove the existing SARIF implementation work at: https://github.com/llvm/llvm-project/blob/main/clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp. Are you still intending to work on that, or has the work from @cjdb changed your plans for the next steps? (I mostly am trying to figure out whether we should land this now because there's enough momentum that it will start being used "Real Soon Now" or whether we should wait for a bit until there's a patch to make use of this and then land the whole patch stack.)



================
Comment at: clang/include/clang/Basic/Sarif.h:82
+public:
+  static SarifArtifactLocation create(const llvm::StringRef URI) {
+    return SarifArtifactLocation{URI.str()};
----------------
We don't typically use top-level `const` like this (same applies elsewhere) unless it's on a pointee/reference.


================
Comment at: clang/include/clang/Basic/Sarif.h:302-305
+  const llvm::StringRef SchemaURI{
+      "https://docs.oasis-open.org/sarif/sarif/v2.1.0/cos02/schemas/"
+      "sarif-schema-2.1.0.json"};
+  const llvm::StringRef SchemaVersion{"2.1.0"};
----------------
FWIW, this use of top-level `const` is fine (we do use it for data member variables).


================
Comment at: clang/include/clang/Basic/Sarif.h:162
+public:
+  static ThreadFlow create() { return {}; }
+
----------------
aaron.ballman wrote:
> One question this raises for me is whether we should be enforcing invariants from the SARIF spec as part of this interface or not. Currently, you can create a thread flow that has no importance or a rule without a name/id, etc. That means it's pretty easy to create SARIF that won't validate properly. One possible way to alleviate this would be for the `create()` methods/ctors to require these be set when creating the objects. However, I can imagine there will be times when that is awkward due to following the builder pattern with the interface. Another option would be to have some validation methods on each of the interfaces and the whole tree gets validated after construction, but this could have performance impacts.
> 
> What are your thoughts?
Are you still intending to add a `validate()` interface to take care of this, or are you still thinking about how to enforce invariants during construction (or some combination of the two)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109701