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

Fri Aug 26 04:41:41 PDT 2022

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<CharSourceRange> &Ranges,
+                       ArrayRef<FixItHint> Hints) override {}
----------------
abrahamcd wrote:
> aaron.ballman wrote:
> > Move this implementation to live with the rest and give it an assertion?
> This function is currently being called from the DiagnosticRenderer class that both Text and SARIFDiagnostics inherit from. Maybe this could be part of the refactoring, making sure that any text-specific function calls are moved to TextDiagnostic rather than being in the general Renderer base class?
Ah, good call, the assert definitely would be wrong then. Let's leave it alone for now, it's fine as-is.

================
Comment at: clang/lib/Frontend/SARIFDiagnosticPrinter.cpp:53
+  OS.flush();
+}
+
----------------
cjdb wrote:
> aaron.ballman wrote:
> > Should we be invalidating that `SARIFDiag` object here so the printer cannot be used after the source file has ended?
> I suggested that we don't do this, because the next run of `BeginSourceFile` can take care of that when making the new `SARIFDiagnostic`. However, if we were to put an assert at the top of every member function other than `BeginSourceFile` to ensure that SARIFDiag isn't null (and one at the top of `BeginSourceFile` to ensure that it _is_), I think there might be good value in re-adding this.
SGTM, thanks for weighing in!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131632