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

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y added a comment.

Hi,

Apologies for the long delay! I was on a short break to focus to other projects. I have some changes in mind such as:

- Creating the `SarifLog` object to represent the top-level document. Currently we store this as a JSON object which ends up rather mucky. Having a separate structure can release internal state in `SarifDocumentWriter`. That way the writer will end up only dealing with the serialization of a `SarifLog`.

Regarding how to validate documents, I think having a `SarifLog::validate()` that checks everything underneath is the way to go. Ideally I'd prefer handling on the interface level, but I'm uncertain what would be a good approach. What do you think?

I'll comb through the previous comments again to make sure I'm not missing punctuations, will signal when this is ready for review again.

Thanks!



================
Comment at: clang/include/clang/Basic/Sarif.h:112
+
+  explicit SarifArtifact(const SarifArtifactLocation &Loc) : Location(Loc) {}
+
----------------
aaron.ballman wrote:
> Should we delete the default ctor?
Agreed, there's no reason for it to be callable. Will do the same for `SarifArtifactLocation`.


================
Comment at: clang/include/clang/Basic/Sarif.h:340
+  /// Create a new empty SARIF document
+  SarifDocumentWriter() : Closed(true){};
+
----------------
aaron.ballman wrote:
> Once you use the default ctor, there's no way to associate language options with the document writer. Is it wise to expose this constructor? (If we didn't, then we could store the `LangOptions` as a const reference instead of making a copy in the other constructor. Given that we never mutate the options, I think that's a win.)
That's a good observation, I will delete this constructor and expose `SarifDocumentWriter(const LangOptions &LangOpts)` instead. 


================
Comment at: clang/lib/Basic/Sarif.cpp:248
+void SarifDocumentWriter::endRun() {
+  // Exit early if trying to close a closed Document
+  if (Closed) {
----------------
aaron.ballman wrote:
> (At this point, I'll stop commenting on these -- can you go through the patch and make sure that all comments have appropriate terminating punctuation?)
Ack, sincere apologies again!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109701