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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
