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

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb accepted this revision.
cjdb added a comment.
This revision is now accepted and ready to land.

Open comments notwithstanding, I'm happy with this patch. Thank you for working on this, it's a huge step towards getting more helpful diagnostics for C++!

@aaron.ballman, @vaibhav.y, do you folks have any further concerns? (@denik has some commentary incoming)



================
Comment at: clang/include/clang/Frontend/SARIFDiagnostic.h:24-26
+  raw_ostream *OS;
+
+  SarifDocumentWriter *Writer;
----------------
These can go in the private section below.


================
Comment at: clang/include/clang/Frontend/SARIFDiagnosticPrinter.h:72
+
+  bool OwnsOutputStream = true;
+};
----------------
This can't ever happen, because there's no constructor that doesn't set `OwnsOutputStream`.


================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:86-88
+    if (Range.isInvalid()) {
+      continue;
+    }
----------------
It seems @aaron.ballman has finally trained me :(


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131632/new/

https://reviews.llvm.org/D131632