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

[Abraham Corea Diaz via Phabricator via cfe-commits]

abrahamcd added inline comments.


================
Comment at: clang/include/clang/Frontend/SARIFDiagnostic.h:45-47
+  void emitCodeContext(FullSourceLoc Loc, DiagnosticsEngine::Level Level,
+                       SmallVectorImpl<CharSourceRange> &Ranges,
+                       ArrayRef<FixItHint> Hints) override {}
----------------
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?


================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:58
+
+  if (Loc.isValid())
+    Result = addLocationToResult(Result, Loc, PLoc, Ranges, *Diag);
----------------
denik wrote:
> I think we should add a test case when Loc or/and Source Manager is not set.
> If Loc is not set SarifDocumentWriter might not create the `locations` property which is required by the spec.
> If this is the case we need to fix it. But I'm not sure if emitDiagnosticMessage() is going to be called when Loc.isInvalid() ).
I believe if Loc is invalid, it goes to one of those special cases I mentioned in this review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131632