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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/Sarif.h:229
+/// used to create an empty shell onto which attributes can be added using the
+/// \c setX(...) methods. The 
+///
----------------
No idea what clang-format wants done here, but there's a stray `The` at the end of the line.


================
Comment at: clang/include/clang/Basic/Sarif.h:95
+//
+/// Since every in clang artifact MUST have a location (there being no nested
+/// artifacts), the creation method \ref SarifArtifact::create requires a
----------------
vaibhav.y wrote:
> aaron.ballman wrote:
> > How do we expect to handle artifact locations that don't correspond directly to a file? For example, the user can specify macros on the command line and those macros could have a diagnostic result associated with them. Can we handle that sort of scenario?
> I hadn't considered `-D` macros. Definitely a valid case to handle. I will respond with more information once I read through the related portions of the SARIF spec. A (sort of hacky?) solution come to mind after a quick glance:
> 
> Setting the artifact URI to: `data:text/plain:<CLI_ARG>` with an offset, and saying that it's role is `"referencedOnCommandLine"`. It seems strange since we need to copy over the command line, instead of referencing it directly. What do you think: https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317617
> 
> Dropping a TODO comment so I can update later.
> 
> `TODO(envp)`
>From my reading of SARIF, I think that sounds plausible. At least, I didn't see anything else that would work.


================
Comment at: clang/lib/Basic/Sarif.cpp:230-232
+  if (!hasRun()) {
+    return;
+  }
----------------
vaibhav.y wrote:
> aaron.ballman wrote:
> > Is there a reason why we don't want to assert instead?
> Creating a document requires ending any ongoing runs, and it is valid to create a document without any runs, so `createDocument()` calls `endRun()`.
> 
> I guess having a flag to mark the status of the document, and only calling `endRun()` if a an active run exists would likely be better. (`hasRun()` seems to have a rather broad responsibility, tracking both the availability & state of the current run)
> 
> I'm thinking of adding a `Closed` flag to the writer (default `true`), which is unset whenever `createRun()` is called, and `endRun()` will set the same flag. That way we only `endRun()` if there is something to end, like so: 
> 
> 1. Constructor makes writer with `Closed = true`
> 2. `createRun()` requires `Closed == true`, and sets it to `false`
> 3. `endRun()` requires `Closed == false`, and sets it to `true`
> 4. `createDocument()` requires `Closed == true`, and will call `endRun()` to ensure that
> 
> What do you think?
I think that makes sense. Adding assertions where appropriate would also help users who misuse the interface catch their issues earlier rather than later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109701