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