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

[Denis Nikitin via Phabricator via cfe-commits]

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 about Writer's ownership and why it's a raw pointer here (like we discussed offline).


================
Comment at: clang/include/clang/Frontend/SARIFDiagnosticPrinter.h:2
+//===--- SARIFDiagnosticPrinter.h - SARIF Diagnostic Client -------*- C++
+//-*-===//
+//
----------------
Please adjust the length to fit it in one line.


================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:2
+//===------- SARIFDiagnostic.cpp - SARIF Diagnostic Formatting
+//-------------===//
+//
----------------
Same here.


================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:38
+    : DiagnosticRenderer(LangOpts, DiagOpts), OS(&OS), Writer(Writer) {}
+
+void SARIFDiagnostic::emitDiagnosticMessage(
----------------
Could you please add FIXME with a pointer to llvm-project/issues/57323 to refactor Diagnostic classes?


================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:58
+
+  if (Loc.isValid())
+    Result = addLocationToResult(Result, Loc, PLoc, Ranges, *Diag);
----------------
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() ).


================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:116
+
+    Locations.push_back(
+        CharSourceRange{SourceRange{BeginLoc, EndLoc}, /* ITR = */ false});
----------------
abrahamcd wrote:
> I noticed that when processing additional source ranges, regular source locations are used instead of the presumed locations. Is this an intentional difference from single location diagnostics? (This issue is present in the current TextDiagnostics as well.)
Put FIXME here (if it's not already somewhere else).


================
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
+      }
----------------
aaron.ballman wrote:
> cjdb wrote:
> > I think it would be good to file an issue on GitHub and change to `FIXME(llvm-project/<issue>)`. @aaron.ballman WDYT?
> I'd be fine with that.
@abrahamcd please file the llvm-project/issues and link it here.


================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:191
+#ifdef _WIN32
+      TmpFilename = (*File)->getName();
+      llvm::sys::fs::make_absolute(TmpFilename);
----------------
cjdb wrote:
> aaron.ballman wrote:
> > Note: this is not a particularly small string when it requires 4k by default.
> There's a bug either here or in the function's interface: the function returns a `StringRef` to a stack object. Do we really need ownership here?
> Note: this is not a particularly small string when it requires 4k by default.

This still has to be addressed.


================
Comment at: clang/lib/Frontend/SARIFDiagnosticPrinter.cpp:2
+//===------- SARIFDiagnosticPrinter.cpp - Diagnostic
+// Printer----------------===//
+//
----------------
Same as above. One line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131632