Change setDiagnosticsOutputFile to take a unique_ptr from a raw pointer (NFC)

[mehdi_amini]

Summary

This makes it explicit that ownership is taken. Also replace all new
with make_unique<> at call sites.

[malcolm.parsons]

llvm/include/llvm/IR/LLVMContext.h

197 ↗

(On Diff #78618)

Pass by value if always moving doesn't apply to move-only types. Use && to avoid a temporary.

[mehdi_amini]

Pushed inadvertently with D26886, I'll address the review as post-commit.

llvm/include/llvm/IR/LLVMContext.h

197 ↗

(On Diff #78618)

Sorry I don't understand this comment?

[malcolm.parsons]

llvm/include/llvm/IR/LLVMContext.h

197 ↗

(On Diff #78618)

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.

[mehdi_amini]

llvm/include/llvm/IR/LLVMContext.h

197 ↗

(On Diff #78618)

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)

[malcolm.parsons]

llvm/include/llvm/IR/LLVMContext.h

197 ↗

(On Diff #78618)

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

[mehdi_amini]

llvm/include/llvm/IR/LLVMContext.h

197 ↗

(On Diff #78618)

Did you end up submitting a patch for the dev guide for move-only type?

[malcolm.parsons]

llvm/include/llvm/IR/LLVMContext.h

197 ↗

(On Diff #78618)

No.
I found
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rf-consume that has an exception for unique_ptr.

[mehdi_amini]

llvm/include/llvm/IR/LLVMContext.h

197 ↗

(On Diff #78618)

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.

cfe/trunk/lib/CodeGen/CodeGenAction.cpp

void HandleTranslationUnit(ASTContext &C) override {
           llvm::make_unique<llvm::tool_output_file>(CodeGenOpts.OptRecordFile,
                                                     EC, sys::fs::F_None);
         if (EC) {
           Diags.Report(diag::err_cannot_open_file) <<
             CodeGenOpts.OptRecordFile << EC.message();
           return;
         }
 
-        Ctx.setDiagnosticsOutputFile(new yaml::Output(OptRecordFile->os()));
+        Ctx.setDiagnosticsOutputFile(
+            llvm::make_unique<yaml::Output>(OptRecordFile->os()));
 
         if (CodeGenOpts.getProfileUse() != CodeGenOptions::ProfileNone)
           Ctx.setDiagnosticHotnessRequested(true);
       }
 
       // Link LinkModule into this module if present, preserving its validity.
       for (auto &I : LinkModules) {
         unsigned LinkFlags = I.first;

llvm/trunk/include/llvm/IR/LLVMContext.h

public:
   /// diagnostics.  If null, diagnostics are not saved in a file but only
   /// emitted via the diagnostic handler.
   yaml::Output *getDiagnosticsOutputFile();
   /// Set the diagnostics output file used for optimization diagnostics.
   ///
   /// By default or if invoked with null, diagnostics are not saved in a file
   /// but only emitted via the diagnostic handler.  Even if an output file is
   /// set, the handler is invoked for each diagnostic message.
-  void setDiagnosticsOutputFile(yaml::Output *F);
+  void setDiagnosticsOutputFile(std::unique_ptr<yaml::Output> F);
 
   /// \brief Get the prefix that should be printed in front of a diagnostic of
   ///        the given \p Severity
   static const char *getDiagnosticMessagePrefix(DiagnosticSeverity Severity);
 
   /// \brief Report a message to the currently installed diagnostic handler.
   ///
   /// This function returns, in particular in the case of error reporting

llvm/trunk/lib/IR/LLVMContext.cpp

 bool LLVMContext::getDiagnosticHotnessRequested() const {
   return pImpl->DiagnosticHotnessRequested;
 }
 
 yaml::Output *LLVMContext::getDiagnosticsOutputFile() {
   return pImpl->DiagnosticsOutputFile.get();
 }
 
-void LLVMContext::setDiagnosticsOutputFile(yaml::Output *F) {
-  pImpl->DiagnosticsOutputFile.reset(F);
+void LLVMContext::setDiagnosticsOutputFile(std::unique_ptr<yaml::Output> F) {
+  pImpl->DiagnosticsOutputFile = std::move(F);
 }
 
 LLVMContext::DiagnosticHandlerTy LLVMContext::getDiagnosticHandler() const {
   return pImpl->DiagnosticHandler;
 }
 
 void *LLVMContext::getDiagnosticContext() const {
   return pImpl->DiagnosticContext;

llvm/trunk/lib/LTO/LTOCodeGenerator.cpp

if (RemarksFilename != "") {
     std::error_code EC;
     DiagnosticOutputFile = llvm::make_unique<tool_output_file>(
         RemarksFilename, EC, sys::fs::F_None);
     if (EC) {
       emitError(EC.message());
       return false;
     }
     Context.setDiagnosticsOutputFile(
-        new yaml::Output(DiagnosticOutputFile->os()));
+        llvm::make_unique<yaml::Output>(DiagnosticOutputFile->os()));
   }
   return true;
 }
 
 void LTOCodeGenerator::finishOptimizationRemarks() {
   if (DiagnosticOutputFile) {
     DiagnosticOutputFile->keep();
     // FIXME: LTOCodeGenerator dtor is not invoked on Darwin

llvm/trunk/tools/opt/opt.cpp

#endif
   if (RemarksFilename != "") {
     std::error_code EC;
     YamlFile = llvm::make_unique<tool_output_file>(RemarksFilename, EC,
                                                    sys::fs::F_None);
     if (EC) {
       errs() << EC.message() << '\n';
       return 1;
     }
-    Context.setDiagnosticsOutputFile(new yaml::Output(YamlFile->os()));
+    Context.setDiagnosticsOutputFile(
+        llvm::make_unique<yaml::Output>(YamlFile->os()));
   }
 
   // Load the input module...
   std::unique_ptr<Module> M = parseIRFile(InputFilename, Err, Context);
 
   if (!M) {
     Err.print(argv[0], errs());
     return 1;