[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/SARIFDiagnosticPrinter.h:63-65
+  void setSarifWriter(SarifDocumentWriter *SarifWriter) {
+    Writer = std::unique_ptr<SarifDocumentWriter>(SarifWriter);
+  }
----------------
abrahamcd wrote:
> aaron.ballman wrote:
> > This interface seems dangerous to me -- the caller has no idea that the pointer will be stashed off as a `unique_ptr` and so the caller might expect to still be able to use its (now not unique) pointer value.
> Would it be best to store as a regular pointer instead of a unique pointer in this case then?
My preference is to use a `unique_ptr` whenever possible so that ownership semantics are clear. So my recommendation is to have the user construct the `unique_ptr` and hand it off through this interface with a `move`.


================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:189
+      // on Windows we can just use llvm::sys::path::remove_dots(), because,
+      // on that system, both aforementioned paths point to the same place.
+#ifdef _WIN32
----------------
abrahamcd wrote:
> aaron.ballman wrote:
> > I'm not certain we want a difference in behavior here. For starters, Windows also has hardlinks and symlinks (they're not exactly the same as on *nix systems though), so I think the assumption these paths point to the same place is valid. But also, platform-specific behavior should be exceedingly rare  in most parts of the compiler. Is there significant harm in unifying the behavior across platforms?
> There seems to be Windows-specific tests that rely on this behavior, but I'm not too sure what the harm in unifying it would be. There's some discussion on it here: D59415. 
Then let's skip this for now -- if there's something we want to address, we can do it in a follow-up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131632