[PATCH] D131632: [clang] Enable output of SARIF diagnostics

aaron.ballman added inline comments. Comment at: clang/test/Frontend/sarif-diagnostics.cpp:8 +// Omit filepath to llvm project directory +// CHECK:

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

aaron.ballman added inline comments. Comment at: clang/test/Frontend/sarif-diagnostics.cpp:8 +// Omit filepath to llvm project directory +// CHECK:

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

tstellar added inline comments. Comment at: clang/test/Frontend/sarif-diagnostics.cpp:8 +// Omit filepath to llvm project directory +// CHECK:

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

aaron.ballman added inline comments. Comment at: clang/test/Frontend/sarif-diagnostics.cpp:8 +// Omit filepath to llvm project directory +// CHECK:

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

uabelho added inline comments. Comment at: clang/test/Frontend/sarif-diagnostics.cpp:8 +// Omit filepath to llvm project directory +// CHECK:

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

dyung added inline comments. Comment at: clang/test/Frontend/sarif-diagnostics.cpp:8 +// Omit filepath to llvm project directory +// CHECK:

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

dyung added inline comments. Comment at: clang/test/Frontend/sarif-diagnostics.cpp:8 +// Omit filepath to llvm project directory +// CHECK:

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

thakis added a comment. This adds warnings: [498/568] CXX obj/clang/lib/Frontend/Frontend.SARIFDiagnostic.o ../../clang/lib/Frontend/SARIFDiagnostic.cpp:74:25: warning: unused variable 'Filename' [-Wunused-variable] llvm::StringRef Filename = ^ In file

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG82e893c47c77: [clang] Enable output of SARIF diagnostics (authored by abrahamcd, committed by cjdb). Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

abrahamcd updated this revision to Diff 455968. abrahamcd added a comment. Reset SARIFDiag during EndSourceFile(). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131632/new/ https://reviews.llvm.org/D131632 Files:

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

abrahamcd updated this revision to Diff 455964. abrahamcd added a comment. Deleted copy and move for Printer and added asserts. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131632/new/ https://reviews.llvm.org/D131632 Files:

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

abrahamcd added inline comments. Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:58 + + if (Loc.isValid()) +Result = addLocationToResult(Result, Loc, PLoc, Ranges, *Diag); denik wrote: > abrahamcd wrote: > > denik wrote: > > > I think we should add a

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

abrahamcd updated this revision to Diff 455955. abrahamcd marked 10 inline comments as done. abrahamcd added a comment. Resolved pending comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131632/new/ https://reviews.llvm.org/D131632 Files:

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM! Comment at: clang/include/clang/Frontend/SARIFDiagnostic.h:45-47 + void emitCodeContext(FullSourceLoc Loc, DiagnosticsEngine::Level Level, + SmallVectorImpl , +

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

denik accepted this revision. denik added inline comments. Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:58 + + if (Loc.isValid()) +Result = addLocationToResult(Result, Loc, PLoc, Ranges, *Diag); abrahamcd wrote: > denik wrote: > > I think we should

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

abrahamcd updated this revision to Diff 455729. abrahamcd added a comment. Ran clang-format. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131632/new/ https://reviews.llvm.org/D131632 Files: clang/include/clang/Frontend/SARIFDiagnostic.h

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

cjdb added inline comments. Comment at: clang/lib/Frontend/SARIFDiagnosticPrinter.cpp:53 + OS.flush(); +} + aaron.ballman wrote: > Should we be invalidating that `SARIFDiag` object here so the printer cannot > be used after the source file has ended? I

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

abrahamcd added inline comments. Comment at: clang/include/clang/Frontend/SARIFDiagnostic.h:45-47 + void emitCodeContext(FullSourceLoc Loc, DiagnosticsEngine::Level Level, + SmallVectorImpl , + ArrayRef Hints) override {}

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

abrahamcd updated this revision to Diff 455728. abrahamcd marked 15 inline comments as done. abrahamcd added a comment. Addressed some refactoring and reformatting comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131632/new/

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

cjdb added inline comments. Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:191 +#ifdef _WIN32 + TmpFilename = (*File)->getName(); + llvm::sys::fs::make_absolute(TmpFilename); denik wrote: > cjdb wrote: > > aaron.ballman wrote: > > > Note: this is

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

aaron.ballman added inline comments. Comment at: clang/include/clang/Frontend/SARIFDiagnostic.h:30-33 + SARIFDiagnostic & operator= ( const SARIFDiagnostic && ) = delete; + SARIFDiagnostic ( SARIFDiagnostic && ) = delete; + SARIFDiagnostic & operator= ( const SARIFDiagnostic

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

denik added inline comments. Comment at: clang/include/clang/Frontend/SARIFDiagnostic.h:24-26 + raw_ostream *OS; + + SarifDocumentWriter *Writer; cjdb wrote: > These can go in the private section below. In addition, I think it's worth putting a comment here

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

abrahamcd updated this revision to Diff 455394. abrahamcd marked 4 inline comments as done. abrahamcd added a comment. Deleted copy/move from renderer and returned OS to reference. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131632/new/

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

aaron.ballman added inline comments. Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:86-88 +if (Range.isInvalid()) { + continue; +} cjdb wrote: > It seems @aaron.ballman has finally trained me :( {meme, src=ohno} My work here is done. ;-)

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

aaron.ballman added inline comments. Comment at: clang/include/clang/Frontend/SARIFDiagnosticPrinter.h:31 +class SARIFDiagnosticPrinter : public DiagnosticConsumer { + raw_ostream + IntrusiveRefCntPtr DiagOpts; cjdb wrote: > cjdb wrote: > > Please make OS a

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

cjdb accepted this revision. cjdb added a comment. This revision is now accepted and ready to land. Open comments notwithstanding, I'm happy with this patch. Thank you for working on this, it's a huge step towards getting more helpful diagnostics for C++! @aaron.ballman, @vaibhav.y, do you

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

abrahamcd updated this revision to Diff 455005. abrahamcd added a comment. Removed use of pointer reset in Printer. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131632/new/ https://reviews.llvm.org/D131632 Files:

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

cjdb added inline comments. Comment at: clang/include/clang/Frontend/SARIFDiagnosticPrinter.h:41 + void setPrefix(llvm::StringRef Value) { +Prefix = std::move(Value); + } We don't need to move a StringRef. Comment at:

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

abrahamcd updated this revision to Diff 454950. abrahamcd marked 2 inline comments as done. abrahamcd added a comment. Added use of std::make_unique. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131632/new/ https://reviews.llvm.org/D131632

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

cjdb added inline comments. Comment at: clang/lib/Frontend/FrontendAction.cpp:727-728 + static_cast(()) + ->setSarifWriter(std::unique_ptr( + new SarifDocumentWriter(CI.getSourceManager(; +} Now that the interface is using

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

abrahamcd added inline comments. Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:116 + +Locations.push_back( +CharSourceRange{SourceRange{BeginLoc, EndLoc}, /* ITR = */ false}); I noticed that when processing additional source ranges, regular

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

abrahamcd updated this revision to Diff 454926. abrahamcd added a comment. Documented issue with representing PresumedLocs modified by \#line directives. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131632/new/ https://reviews.llvm.org/D131632

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

abrahamcd updated this revision to Diff 454918. abrahamcd marked 4 inline comments as done. abrahamcd added a comment. Modified Printer's interface to use unique pointers when setting the SARIF writer. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

aaron.ballman added inline comments. Comment at: clang/include/clang/Frontend/SARIFDiagnosticPrinter.h:63-65 + void setSarifWriter(SarifDocumentWriter *SarifWriter) { +Writer = std::unique_ptr(SarifWriter); + } abrahamcd wrote: > aaron.ballman wrote: > >

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

abrahamcd added inline comments. Comment at: clang/include/clang/Frontend/SARIFDiagnosticPrinter.h:63-65 + void setSarifWriter(SarifDocumentWriter *SarifWriter) { +Writer = std::unique_ptr(SarifWriter); + } aaron.ballman wrote: > This interface seems

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

abrahamcd updated this revision to Diff 454652. abrahamcd marked 26 inline comments as done. abrahamcd added a comment. Addressed review comments on formatting, style, C++ best practices. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131632/new/

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

denik added inline comments. Comment at: clang/lib/Frontend/SARIFDiagnosticPrinter.cpp:71 + // other infrastructure necessary when emitting more rich diagnostics. + if (!Info.getLocation().isValid()) { // TODO: What is this case? +//

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

aaron.ballman added inline comments. Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:78 +emitFilename(FE->getName(), Loc.getManager()); +// FIXME: No current way to add file-only location to SARIF object + } cjdb wrote: > I think it

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

cjdb added a comment. This looks great, thank you so much @abrahamcd! Comment at: clang/include/clang/Frontend/SARIFDiagnosticPrinter.h:31 +class SARIFDiagnosticPrinter : public DiagnosticConsumer { + raw_ostream + IntrusiveRefCntPtr DiagOpts; Please make

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

aaron.ballman added inline comments. Comment at: clang/include/clang/Frontend/SARIFDiagnosticPrinter.h:42 + + unsigned OwnsOutputStream : 1; + There's not a lot of benefit to using bit-fields here yet, so I'd make this field a `bool` instead for the time

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

abrahamcd updated this revision to Diff 454129. abrahamcd edited the summary of this revision. abrahamcd added a comment. Addressed review style comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131632/new/ https://reviews.llvm.org/D131632

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

denik added inline comments. Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:155 +break; + } +} vaibhav.y wrote: > Does this need an `llvm_unreachable` after the switch? I guess no, because of `break`s. But... Although `case DiagnosticsEngine::Ignored`

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

abrahamcd updated this revision to Diff 454113. abrahamcd marked 2 inline comments as done. abrahamcd added a comment. Added diagnostic level and default configuration to SARIF output. Removed unit test file. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

denik added a comment. Thanks Abraham! Please mention in the summary that some part of SARIF Diag implementation was copied from Text Diag and further refactoring is needed. Also, like we discussed: - drop the unittest; - replace strict match of json output in FileCheck with partial matches

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

abrahamcd added a comment. In D131632#3733068 , @aaron.ballman wrote: > Precommit CI found a relevant failure. I think this was just from the unfinished unit test. I can go ahead and remove `clang/unittests/Frontend/SARIFDiagnosticTest.cpp` entirely

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

abrahamcd updated this revision to Diff 453788. abrahamcd added a comment. Commented out unfinished test case Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131632/new/ https://reviews.llvm.org/D131632 Files:

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

aaron.ballman added a comment. Precommit CI found a relevant failure. Comment at: clang/lib/Frontend/SARIFDiagnosticPrinter.cpp:71 + // other infrastructure necessary when emitting more rich diagnostics. + if (!Info.getLocation().isValid()) { // TODO: What is this case? +

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

vaibhav.y added inline comments. Comment at: clang/lib/Frontend/SARIFDiagnosticPrinter.cpp:71 + // other infrastructure necessary when emitting more rich diagnostics. + if (!Info.getLocation().isValid()) { // TODO: What is this case? +//

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

vaibhav.y added inline comments. Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:155 +break; + } +} Does this need an `llvm_unreachable` after the switch? Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:177 + // original path

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

abrahamcd updated this revision to Diff 453346. abrahamcd added a comment. Removed Clang name from FileCheck test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131632/new/ https://reviews.llvm.org/D131632 Files:

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

vaibhav.y added inline comments. Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:75 +emitFilename(FE->getName(), Loc.getManager()); +// FIXME: No current way to add file-only location to SARIF object + } abrahamcd wrote: > @vaibhav.y

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

abrahamcd updated this revision to Diff 453126. abrahamcd added a comment. Fixed FileCheck test case and added multiple source range error. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131632/new/ https://reviews.llvm.org/D131632 Files:

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

abrahamcd added inline comments. Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:75 +emitFilename(FE->getName(), Loc.getManager()); +// FIXME: No current way to add file-only location to SARIF object + } @vaibhav.y , just wanted to

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

abrahamcd updated this revision to Diff 452843. abrahamcd added a comment. Naming fixes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131632/new/ https://reviews.llvm.org/D131632 Files: clang/include/clang/Frontend/SARIFDiagnostic.h

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

abrahamcd updated this revision to Diff 452840. abrahamcd added a comment. Minor cleanup. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131632/new/ https://reviews.llvm.org/D131632 Files: clang/include/clang/Frontend/SARIFDiagnostic.h

[PATCH] D131632: [clang] Enable output of SARIF diagnostics

abrahamcd updated this revision to Diff 452833. abrahamcd retitled this revision from "[WIP] Enable SARIF Diagnostics" to "[clang] Enable output of SARIF diagnostics". abrahamcd edited the summary of this revision. abrahamcd added a comment. Removed unused remaining parts of traditional text