[PATCH] D26884: Change setDiagnosticsOutputFile to take a unique_ptr from a raw pointer (NFC)

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: llvm/include/llvm/IR/LLVMContext.h:197
   /// set, the handler is invoked for each diagnostic message.
-  void setDiagnosticsOutputFile(yaml::Output *F);
+  void setDiagnosticsOutputFile(std::unique_ptr<yaml::Output> F);
 
----------------
malcolm.parsons wrote:
> mehdi_amini wrote:
> > malcolm.parsons wrote:
> > > mehdi_amini wrote:
> > > > malcolm.parsons wrote:
> > > > > mehdi_amini wrote:
> > > > > > malcolm.parsons wrote:
> > > > > > > Pass by value if always moving doesn't apply to move-only types. Use && to avoid a temporary.
> > > > > > Sorry I don't understand this comment?
> > > > > Functions that accept a parameter by value do so to allow the caller to choose whether to supply an lvalue that is copied or an rvalue that is moved without having to write overloads for both.
> > > > > For a move-only type there is no choice, so the function should accept an rvalue.
> > > > > This might avoid the construction and destruction of a temporary.
> > > > I see. It does not really sound idiomatic to me to have any unique_ptr passed by rvalue, you should try to submit a patch to the developer guide.
> > > > 
> > > > (I'm not really convinced by the "avoid the construction and destruction of a temporary" here, I don't expect the codegen to be any different)
> > > There are a few places where unique_ptrs are passed by rvalue:
> > > ```
> > > llvm/Transforms/Utils/MemorySSA.h
> > > 681:    Result(std::unique_ptr<MemorySSA> &&MSSA) : MSSA(std::move(MSSA)) {}
> > > 
> > > llvm/Support/TargetRegistry.h
> > > 77:                                 std::unique_ptr<MCRelocationInfo> &&RelInfo);
> > > 112:      TargetMachine &TM, std::unique_ptr<MCStreamer> &&Streamer);
> > > 157:      std::unique_ptr<MCRelocationInfo> &&RelInfo);
> > > 396:                               std::unique_ptr<MCStreamer> &&Streamer) const {
> > > 531:                     std::unique_ptr<MCRelocationInfo> &&RelInfo) const {
> > > 1125:                               std::unique_ptr<MCStreamer> &&Streamer) {
> > > 
> > > llvm/Bitcode/BitcodeReader.h
> > > 92:  getOwningLazyBitcodeModule(std::unique_ptr<MemoryBuffer> &&Buffer,
> > > ```
> > > but there are lots of places it's passed by value.
> > > 
> > > There's usually no difference in codegen, but not always:
> > > https://godbolt.org/g/8GoljO
> > > https://godbolt.org/g/CNZzsS
> > Did you end up submitting a patch for the dev guide for move-only type?
> No.
> I found
> https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rf-consume that has an exception for unique_ptr.
We could still add a guideline for "move-only types other than unique_ptr", however it might not be frequent enough that it is worth codifying it.


Repository:
  rL LLVM

https://reviews.llvm.org/D26884