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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D109701#3376127 <https://reviews.llvm.org/D109701#3376127>, @vaibhav.y wrote:

> Hi,
>
> Apologies for the long delay! I was on a break to focus to other projects.

Not a problem at all!

> 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`.

That seems like an interesting idea (though I think it could be done in a follow-up).

> 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 think that's a reasonable idea for assert builds to let us know if there are issues with the internal consistency of the data; we do something similar for IR verification in LLVM. But the kind of validating that will tell us whether this is successful is validating against another tool (the whole point to SARIF is to be an exchange format between tools, so self-testing only gets you so much information about how well the implementation works). However, there's no good way to automate that kind of testing in our test suite, so this is more of a request to try the output in tools that can consume SARIF to see if they're successful, and if they're not, see if we can come up with unit tests for those cases.

> 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/lib/Basic/Sarif.cpp:248
+void SarifDocumentWriter::endRun() {
+  // Exit early if trying to close a closed Document
+  if (Closed) {
----------------
vaibhav.y wrote:
> 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!
No worries! These sort of nits are really easy to miss, it happens to me too. :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109701