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