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

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y marked an inline comment as done.
vaibhav.y added a comment.

Thanks, will push changes to address the comments soon.

As I understood from our discussion the work @cjdb has planned would create a new `DiagnosticsConsumer`, it can be started in parallel but would need the changes in D109701 <https://reviews.llvm.org/D109701> to complete. I have other work planned for SARIF as well, some notes here: https://gist.github.com/envp/6abc3230dcc5043416c86aefb3d24419 (@cjdb plans to address the "BRIDGE" component)

For validation, I think having a hybrid approach is best here. As much as possible we should try to be correct by construction, but we certainly need to have validation before we serialize on the data.

My plan is to iterate on the interface as follows:

1. To decompose `SarifDocumentWriter`, into: `SarifLog` & other builders, along with a `SarifLogSerializer` whose sole responsibility is to construct the `json::Value`. This should improve the number of properties that are correct by construction.
2. Try to use this newer interface with https://github.com/llvm/llvm-project/blob/main/clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp, and incorporate any new features I think might be useful

Do you think it would it better to use the changes here in `libStaticAnalyzer` before they land & then iterate on it? I don't have a strong preference for either approach so I'd defer to your experience.

I'm also unfamiliar with the workflow for stacking patches in phabricator, is there documentation I can refer to for this? My guess is: I make a git branch whose base is that of `D109701`, and then `arc diff D109701..OTHER_BRANCH` to create the phabricator that would use these changes?



================
Comment at: clang/include/clang/Basic/Sarif.h:162
+public:
+  static ThreadFlow create() { return {}; }
+
----------------
aaron.ballman wrote:
> 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)?
Definitely! I plan to add that in a follow-up patch. The goal would be to have as much as possible be correct by construction (through small builders having a limited field set), but we'll still need a `validate()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109701