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

Wed Aug 24 14:30:45 PDT 2022

aaron.ballman added inline comments.

================
Comment at: clang/include/clang/Frontend/SARIFDiagnosticPrinter.h:31
+class SARIFDiagnosticPrinter : public DiagnosticConsumer {
+  raw_ostream &OS;
+  IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts;
----------------
cjdb wrote:
> cjdb wrote:
> > Please make OS a pointer instead of a reference, since member references make usage very unpleasant.
> Nit: please move all private members below the public interface, where possible.
> Please make OS a pointer instead of a reference, since member references make usage very unpleasant.

Err... I disagree (and just deleted a comment above asking for `OS` to turn into a reference rather than a pointer because it can never be null). I think we want to use a reference here because 1) it conveys the correct "I can never be null" semantics so maintainers don't have to ask when that can happen, and 2) copies of this class are a bad idea to begin with (we should probably delete the copy and move operations).

We use reference members elsewhere: https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Sema/Sema.h#L407 (not that I would hold Sema up as an example of best practices, lol).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131632