aaron.ballman added inline comments.
================
Comment at: clang/include/clang/Frontend/SARIFDiagnostic.h:30-33
+ SARIFDiagnostic & operator= ( const SARIFDiagnostic && ) = delete;
+ SARIFDiagnostic ( SARIFDiagnostic && ) = delete;
+ SARIFDiagnostic & operator= ( const SARIFDiagnostic & ) = delete;
+ SARIFDiagnostic ( const SARIFDiagnostic & ) = delete;
----------------
Formatting nits.
================
Comment at: clang/include/clang/Frontend/SARIFDiagnostic.h:45-47
+ void emitCodeContext(FullSourceLoc Loc, DiagnosticsEngine::Level Level,
+ SmallVectorImpl<CharSourceRange> &Ranges,
+ ArrayRef<FixItHint> Hints) override {}
----------------
Move this implementation to live with the rest and give it an assertion?
================
Comment at: clang/include/clang/Frontend/SARIFDiagnosticPrinter.h:72
+
+ bool OwnsOutputStream;
+};
----------------
This can go away, this never owns the output stream.
================
Comment at: clang/lib/Frontend/SARIFDiagnosticPrinter.cpp:42
+ // Build the SARIFDiagnostic utility.
+ assert(hasSarifWriter() && "Writer not set!");
+ SARIFDiag = std::make_unique<SARIFDiagnostic>(OS, LO, &*DiagOpts, &*Writer);
----------------
Should we also assert that we don't already have a valid `SARIFDiag` object (in
case someone calls `BeginSourceFile()` twice)?
================
Comment at: clang/lib/Frontend/SARIFDiagnosticPrinter.cpp:53
+ OS.flush();
+}
+
----------------
Should we be invalidating that `SARIFDiag` object here so the printer cannot be
used after the source file has ended?
================
Comment at: clang/lib/Frontend/SARIFDiagnosticPrinter.cpp:35-36
+SARIFDiagnosticPrinter::~SARIFDiagnosticPrinter() {
+ if (OwnsOutputStream)
+ delete &OS;
+}
----------------
cjdb wrote:
> This makes me very uncomfortable. @aaron.ballman do we have a smart-pointer
> that works like `variant<T*, unique_ptr<T>>`?
No, but we shouldn't need it -- this object *never* owns the output stream, so
I think we should remove the member variable and this code (we can default the
dtor then).
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
