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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
