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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
