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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG4b03ad650645: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface (authored by vaibhav.y, committed by aaron.ballman).

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

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM assuming precommit CI comes back happy with it, thank you! I'll land it once I see things are green. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109701/new/

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

vaibhav.y updated this revision to Diff 444960. vaibhav.y added a comment. Undo test case renames Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109701/new/ https://reviews.llvm.org/D109701 Files: clang/include/clang/Basic/Sarif.h

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

vaibhav.y updated this revision to Diff 444764. vaibhav.y added a comment. Gate death tests on `NDEBUG` and available of `GTEST_HAS_DEATH_TEST` This should fix recent pre-merge failures Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109701/new/

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

vaibhav.y added a comment. @aaron.ballman The culprit turned out to be the difference in release flags on the build server vs my environment. I had unfortunately run the configuration command once in `Debug` mode, and hadn't re-configured. Not a bright moment :) The `ASSERT_DEATH` tests that

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

abrahamcd added a comment. In D109701#3648168 , @vaibhav.y wrote: > In D109701#3646856 , @abrahamcd > wrote: > >> Hi! I'm interning with @cjdb and @denik this summer and I was working on >> adding a

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

vaibhav.y added a comment. In D109701#3646856 , @abrahamcd wrote: > Hi! I'm interning with @cjdb and @denik this summer and I was working on > adding a `-fdiagnostics-format=sarif` option to start off my project, but I > just found that a previous

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

abrahamcd added a subscriber: denik. abrahamcd added a comment. Hi! I'm interning with @cjdb and @denik this summer and I was working on adding a `-fdiagnostics-format=sarif` option to start off my project, but I just found that a previous abandoned version of this change (D109697

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

aaron.ballman added a comment. In D109701#3642786 , @vaibhav.y wrote: > In D109701#3642748 , @aaron.ballman > wrote: > >> Unfortunately, it still seems to be causing failures (I had to revert again): >> >>

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

vaibhav.y added a comment. In D109701#3642748 , @aaron.ballman wrote: > Unfortunately, it still seems to be causing failures (I had to revert again): > > https://lab.llvm.org/buildbot/#/builders/91/builds/11840 > > It looks to be the same failure as

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

aaron.ballman reopened this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Unfortunately, it still seems to be causing failures (I had to revert again): https://lab.llvm.org/buildbot/#/builders/91/builds/11840 It looks to be the same failure as before

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG69fcf4fd5a01: Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface (authored by vaibhav.y, committed by aaron.ballman).

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

vaibhav.y added a comment. >> I have suspicions that it was the `SarifResult &&` type in the test so I >> changed it to a `const SarifResult &`. >> >> I've tried running it on a RHEL 7, Darwin on my end. > > If you think you've got it fixed, I think the best we can do is to re-commit > and

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

aaron.ballman added a comment. In D109701#3642561 , @vaibhav.y wrote: > Discard most likely culprit in test code causing unexpected crash. > > @aaron.ballman I was unable to reproduce the issue on my end using the > buildbot instructions and ASAN,

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

vaibhav.y updated this revision to Diff 443654. vaibhav.y added a comment. Discard most likely culprit in test code causing unexpected crash. @aaron.ballman I was unable to reproduce the issue on my end using the buildbot instructions and ASAN, UBSAN. Is it possible for you to trigger a

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

vaibhav.y added a comment. In D109701#3622983 , @aaron.ballman wrote: > I had to roll it back because of failures with test bots: > > https://lab.llvm.org/buildbot/#/builders/91/builds/11328 > > So this was reverted in

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

aaron.ballman added a comment. I had to roll it back because of failures with test bots: https://lab.llvm.org/buildbot/#/builders/91/builds/11328 So this was reverted in b46ad1b5be694feefabd4c6cd112cbbd04a7b3a7 , can you

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG329fae7103d3: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface (authored by vaibhav.y, committed by aaron.ballman).

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

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Thank you for the fixes to this! I'll land this again on your behalf with the same information as before. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

vaibhav.y updated this revision to Diff 441130. vaibhav.y added a comment. Assert that `CharSourceRange`s passed to `Threadflow`, `SarifResult` are character Ranges. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109701/new/

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

vaibhav.y added inline comments. Comment at: clang/include/clang/Basic/Sarif.h:167 + + ThreadFlow setRange(const CharSourceRange ) { +Range = ItemRange; aaron.ballman wrote: > Should we assert this source range is not a token range? I don't have a strong

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

aaron.ballman added a comment. Thanks, I think this is heading in the right direction! Comment at: clang/include/clang/Basic/Sarif.h:167 + + ThreadFlow setRange(const CharSourceRange ) { +Range = ItemRange; Should we assert this source range is not a

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

vaibhav.y updated this revision to Diff 440374. vaibhav.y added a comment. Discard include: `clang/Lex/Lexer.h` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109701/new/ https://reviews.llvm.org/D109701 Files: clang/include/clang/Basic/Sarif.h

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

vaibhav.y updated this revision to Diff 440371. vaibhav.y added a comment. Use `CharSourceRange` instead of FullSourceRange so we no longer need to compute the length of the last token. This should already be encoded in the end location of the `CharSourceRange` by the caller TODO(@vaibhav.y):

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

vaibhav.y added inline comments. Comment at: clang/lib/Basic/Sarif.cpp:161 +Region["endColumn"] = adjustColumnPos( +R.getEnd(), Lexer::MeasureTokenLength(R.getEnd().getLocWithOffset(0), + R.getEnd().getManager(), LO));

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

aaron.ballman added inline comments. Comment at: clang/lib/Basic/Sarif.cpp:161 +Region["endColumn"] = adjustColumnPos( +R.getEnd(), Lexer::MeasureTokenLength(R.getEnd().getLocWithOffset(0), + R.getEnd().getManager(), LO));

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

vaibhav.y marked an inline comment as not done. vaibhav.y added a comment. Comment isn't done. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109701/new/ https://reviews.llvm.org/D109701 ___ cfe-commits

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

vaibhav.y added inline comments. Comment at: clang/lib/Basic/Sarif.cpp:161 +Region["endColumn"] = adjustColumnPos( +R.getEnd(), Lexer::MeasureTokenLength(R.getEnd().getLocWithOffset(0), + R.getEnd().getManager(), LO));

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

aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/Sarif.h:296-297 +/// The SourceLocation provided must point at the beginning of the token. +typedef std::function +TokenLenMetric; + I worry about the performance aspects of using a

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

vaibhav.y updated this revision to Diff 440279. vaibhav.y added a comment. Discard unused includes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109701/new/ https://reviews.llvm.org/D109701 Files: clang/include/clang/Basic/Sarif.h

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

vaibhav.y added inline comments. Comment at: clang/lib/Basic/Sarif.cpp:161 +Region["endColumn"] = adjustColumnPos( +R.getEnd(), Lexer::MeasureTokenLength(R.getEnd().getLocWithOffset(0), + R.getEnd().getManager(), LO));

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

vaibhav.y updated this revision to Diff 440270. vaibhav.y added a comment. Factor dependency on `Lexer::MeasureTokenLength` into externally provided functor Introduces a type: `TokenLengthMetric` which measures the length of a token starting at the given SLoc Repository: rG LLVM Github

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

aaron.ballman added inline comments. Comment at: clang/lib/Basic/Sarif.cpp:161 +Region["endColumn"] = adjustColumnPos( +R.getEnd(), Lexer::MeasureTokenLength(R.getEnd().getLocWithOffset(0), + R.getEnd().getManager(), LO));

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

vaibhav.y added inline comments. Comment at: clang/lib/Basic/Sarif.cpp:161 +Region["endColumn"] = adjustColumnPos( +R.getEnd(), Lexer::MeasureTokenLength(R.getEnd().getLocWithOffset(0), + R.getEnd().getManager(), LO));

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

aaron.ballman requested changes to this revision. aaron.ballman added a comment. This revision now requires changes to proceed. Marking as needing changes so it's clear this shouldn't be re-landed yet. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

aaron.ballman reopened this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Unfortunately, I had to roll this back in 7a3918b540c30cc630aaae9124c67e5e4db123c2 because there's a

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG6546fdbe36fd: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface (authored by vaibhav.y, committed by aaron.ballman).

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

vaibhav.y added a comment. >> I will update the commit messages, but I cannot commit to the github repo. > > Ah, thank you for letting me know. I can land the changes on your behalf. > What name and email address would you like me to use for patch attribution? > (I probably won't land it until

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

vaibhav.y updated this revision to Diff 439507. vaibhav.y added a comment. Reword commit messages - Prefix `[tag]` specifying which components are affected - Append link to D109701 in commit message per commit message guidelines Repository: rG LLVM Github

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

aaron.ballman added a comment. In D109701#3606042 , @vaibhav.y wrote: > Thanks for your patience with the review as well! Likewise! > Just noticed that I need to add a link to revision in the commit messages as > well:

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

vaibhav.y added a comment. Thanks for your patience with the review as well! Just noticed that I need to add a link to revision in the commit messages as well: (https://www.llvm.org/docs/Phabricator.html#committing-a-change) I will update the commit messages, but I cannot commit to the github

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

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Okay, thanks both for your perspective on this. This LGTM as-is and we can handle validation and integration in subsequent patches. Thank you for this, @vaibhav.y! Repository:

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

vaibhav.y updated this revision to Diff 439153. vaibhav.y added a comment. Fix formatting in Sarif.h Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109701/new/ https://reviews.llvm.org/D109701 Files: clang/include/clang/Basic/Sarif.h

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

cjdb added a subscriber: abrahamcd. cjdb added a comment. I think this CL (which is quite large) might be worth just getting the functionality in, and deferring the in-tree usage to a second CL. That second CL probably should just lift the existing diagnostics so that they're in the message

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

vaibhav.y updated this revision to Diff 439096. vaibhav.y added a comment. Discard top-level const specifier where target isn't pointee/ref Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109701/new/ https://reviews.llvm.org/D109701 Files:

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

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

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

aaron.ballman added a comment. Aside from some minor nits and the open question about validation, I think this is getting pretty close. I'm curious about the next steps with this though, given that there are no in-tree uses for it currently. You had mentioned you planned to work on an adapter

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

vaibhav.y updated this revision to Diff 436110. vaibhav.y marked 2 inline comments as done. vaibhav.y added a comment. Fix bug detected from ASAN run Passes `-fsanitize=address` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109701/new/

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

vaibhav.y updated this revision to Diff 436030. vaibhav.y added a comment. Rebase on main branch from upstream git Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109701/new/ https://reviews.llvm.org/D109701 Files:

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

vaibhav.y added a comment. https://discourse.llvm.org/t/rfc-improving-clang-s-diagnostics/62584/8 There is an ongoing RFC similar to the work here, worth noting. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109701/new/

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

vaibhav.y added a comment. @aaron.ballman Would it be possible that I add `::validate` through a follow-up PR? I'm currently checking the JSON output from the writer using Microsoft's online validator , and it is passing. Though it tends to

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

vaibhav.y updated this revision to Diff 434802. vaibhav.y added a comment. Update tests to check serialization as well - SARIF text generated is validated externally against [Microsoft's online validator][1] [1]: https://sarifweb.azurewebsites.net/Validation Repository: rG LLVM Github

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

aaron.ballman added a comment. In 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

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

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.

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

vaibhav.y updated this revision to Diff 414743. vaibhav.y marked 15 inline comments as done. vaibhav.y added a comment. Herald added a project: All. Fixup comments Mark clang::FullSourceRange as returning const & to its locs rebase on upstream main Repository: rG LLVM Github Monorepo CHANGES

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

aaron.ballman added a comment. Thank you for your patience! I finally had the chance to go through this a bit more. I identified a bunch of tiny nits (the review feedback may look scary, but most should be trivial changes), but a few larger things about the design as well that are worth

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

vaibhav.y added a comment. ping: Requesting review 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

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

vaibhav.y added a comment. ping: This is ready for review now. Thanks for your patience with the review as well! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109701/new/ https://reviews.llvm.org/D109701

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

vaibhav.y marked 11 inline comments as done. vaibhav.y added a comment. Mark completed comments as done Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109701/new/ https://reviews.llvm.org/D109701 ___

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

vaibhav.y updated this revision to Diff 390448. vaibhav.y added a comment. Rename enum members Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109701/new/ https://reviews.llvm.org/D109701 Files: clang/include/clang/Basic/Sarif.h

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

vaibhav.y added inline comments. Comment at: clang/include/clang/Basic/Sarif.h:80-82 + static SarifArtifactLocation create(StringRef URI) { +return SarifArtifactLocation{URI}; + } aaron.ballman wrote: > One thing that's worth calling out is that

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

vaibhav.y updated this revision to Diff 390396. vaibhav.y marked 6 inline comments as done. vaibhav.y added a comment. Rebase on upstream/main: - [clangBasic] Format code - [clangBasic] Mark all constructors taking single values `explicit` - [clangBasic] Convert `StringRef` to `std::string`

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

aaron.ballman added a comment. Thanks for your patience with this review! I'm currently in WG14 meetings this week and out on vacation next week, so my review availability is a bit limited at the moment (sorry for that). I think this is heading in the right direction, but there are some

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

vaibhav.y added a comment. Ping. `clang-format` is the only check failing as of now (attempting to reformat the `` links in doc comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109701/new/ https://reviews.llvm.org/D109701

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

vaibhav.y updated this revision to Diff 385813. vaibhav.y added a comment. Format code using patch from buildkite Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109701/new/ https://reviews.llvm.org/D109701 Files:

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

vaibhav.y updated this revision to Diff 385636. vaibhav.y added a comment. Format code Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109701/new/ https://reviews.llvm.org/D109701 Files: clang/include/clang/Basic/Sarif.h

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

vaibhav.y updated this revision to Diff 385634. vaibhav.y added a comment. Add `Closed` to signal if document is writable Introduce a flag `Closed` to `clang::SarifDocumentWriter` that tracks if the underlying sarif document is accepting results to record. i.e. it is in the middle of an

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

vaibhav.y updated this revision to Diff 385609. vaibhav.y added a comment. Summary of changes: - [clangBasic] Fix SIGSEGV in Sarif builders - [clangBasic] Fixup documentation typo, add explicit type Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

tra resigned from this revision. tra added a comment. I'm not sure how/why I ended up as a reviewer here as I don't have much to do with diags, other than adding one now and then. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109701/new/

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

aaron.ballman added reviewers: jranieri-grammatech, tra. aaron.ballman added a comment. Adding some more reviewers who may have an interest in SARIF as an exchange format and know more about the use cases that are important to them. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

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

aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/Sarif.h:229 +/// used to create an empty shell onto which attributes can be added using the +/// \c setX(...) methods. The +/// No idea what clang-format wants done here, but there's a

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

vaibhav.y added inline comments. Comment at: clang/include/clang/Basic/Sarif.h:95 +// +/// Since every in clang artifact MUST have a location (there being no nested +/// artifacts), the creation method \ref SarifArtifact::create requires a aaron.ballman wrote: >

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

vaibhav.y updated this revision to Diff 375734. vaibhav.y marked 34 inline comments as done. vaibhav.y added a comment. Address comments from upstream review: - Mark anonymous functions static, improve documentation - Coding style fixes - Use Ref types as they are intended (as non-owning

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

aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/Sarif.h:74 + + llvm::Optional Index; + StringRef URI; You have a `using namespace llvm;` at the top of the file, so all these `llvm::` nested name specifiers can be removed.

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

vaibhav.y updated this revision to Diff 372548. vaibhav.y added a comment. Format clang/lib/Basic/Sarif.cpp Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109701/new/ https://reviews.llvm.org/D109701 Files: clang/include/clang/Basic/Sarif.h

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

vaibhav.y updated this revision to Diff 372543. vaibhav.y added a comment. Fixup name of class defined in documentation Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109701/new/ https://reviews.llvm.org/D109701 Files:

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

vaibhav.y updated this revision to Diff 372542. vaibhav.y added a comment. Address review comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109701/new/ https://reviews.llvm.org/D109701 Files: clang/include/clang/Basic/Sarif.h

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

vaibhav.y added inline comments. Comment at: clang/lib/Basic/Sarif.cpp:1 +#include "clang/Basic/Sarif.h" +#include "clang/Basic/LangOptions.h" lattner wrote: > THis nees the standard header boilerplate per the coding standards doc Ack, didn't grok the "all

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

vaibhav.y added a comment. > Btw, is the intent for this functionality to replace what's in > SarifDiagnostics.cpp > (https://github.com/llvm/llvm-project/blob/main/clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp) > fairly immediately? Or are we going to carry both implementations? This

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

aaron.ballman edited reviewers, added: aaron.ballman; removed: rsmith. aaron.ballman added a subscriber: lattner. aaron.ballman added a comment. In D109701#2997892 , @lattner wrote: > I'm not sure who the best person is to review this, but it isn't me

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

lattner resigned from this revision. lattner edited reviewers, added: rsmith; removed: lattner. lattner added a comment. I'm not sure who the best person is to review this, but it isn't me anymore sadly. Richard, can you recommend someone? Comment at:

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

vaibhav.y added inline comments. Comment at: clang/include/clang/Basic/Sarif.h:69 +/// Reference: +/// 1. https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317427;>artifactLocation object +/// 2. \ref SarifArtifact I'm not sure how to

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

vaibhav.y updated this revision to Diff 372289. vaibhav.y added a comment. [clangBasic] Fixup header guard Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109701/new/ https://reviews.llvm.org/D109701 Files: clang/include/clang/Basic/Sarif.h

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

vaibhav.y created this revision. vaibhav.y added a reviewer: lattner. Herald added subscribers: dexonsmith, wenlei, mgorny. vaibhav.y edited the summary of this revision. vaibhav.y added a project: clang. vaibhav.y added subscribers: aaron.ballman, lebedev.ri. vaibhav.y added a comment. vaibhav.y