aaron.ballman accepted this revision.
aaron.ballman added a comment.
LGTM!
================
Comment at: clang/include/clang/Frontend/SARIFDiagnostic.h:45-47
+ void emitCodeContext(FullSourceLoc Loc, DiagnosticsEngine::Level Level,
+ SmallVectorImpl<CharSourceRange> &Ranges,
+ ArrayRef<FixItHint> Hints) override {}
----------------
abrahamcd wrote:
> 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?
Ah, good call, the assert definitely would be wrong then. Let's leave it alone
for now, it's fine as-is.
================
Comment at: clang/lib/Frontend/SARIFDiagnosticPrinter.cpp:53
+ OS.flush();
+}
+
----------------
cjdb wrote:
> aaron.ballman wrote:
> > Should we be invalidating that `SARIFDiag` object here so the printer
> > cannot be used after the source file has ended?
> I suggested that we don't do this, because the next run of `BeginSourceFile`
> can take care of that when making the new `SARIFDiagnostic`. However, if we
> were to put an assert at the top of every member function other than
> `BeginSourceFile` to ensure that SARIFDiag isn't null (and one at the top of
> `BeginSourceFile` to ensure that it _is_), I think there might be good value
> in re-adding this.
SGTM, thanks for weighing in!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D131632/new/
https://reviews.llvm.org/D131632
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
