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

[Aaron Ballman via Phabricator via cfe-commits]

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