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

[Abraham Corea Diaz via Phabricator via cfe-commits]

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 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.
> If it does then "if" is redundant here.
Looked closer and the base Renderer calls this function in both cases when Loc is valid and when it is invalid, so the extra check is necessary. In the case that Loc is invalid, you're right that results will not have locations. This might be something to add to the Writer, I couldn't find a way to add an empty locations object.


================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:191
+#ifdef _WIN32
+      TmpFilename = (*File)->getName();
+      llvm::sys::fs::make_absolute(TmpFilename);
----------------
cjdb wrote:
> denik wrote:
> > 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.
> > 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?
> 
> Disregard, I misread `TmpFilename` as `Filename` and thought we were returning a dangling reference. That isn't the case, so there is no bug.
Made it smaller for now, but this should be revisited during refactoring, since text diag still uses 4096


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131632