Merge topic 'modernize-memory-management'

f964739ead cmCTestRunTest: modernize memory management

Acked-by: Kitware Robot <kwrobot@kitware.com>
Merge-request: !4454

6 files changed, 100 insertions, 60 deletions

diff --git a/Source/CTest/cmCTestMultiProcessHandler.cxx b/Source/CTest/cmCTestMultiProcessHandler.cxx
index 2192843ecd..50c963d948 100644
--- a/Source/CTest/cmCTestMultiProcessHandler.cxx
+++ b/Source/CTest/cmCTestMultiProcessHandler.cxx
@@ -12,13 +12,13 @@
 #include <iomanip>
 #include <iostream>
 #include <list>
-#include <memory>
 #include <sstream>
 #include <stack>
 #include <unordered_map>
 #include <utility>
 #include <vector>
 
+#include <cm/memory>
 #include <cmext/algorithm>
 
 #include "cmsys/FStream.hxx"
@@ -172,7 +172,8 @@ bool cmCTestMultiProcessHandler::StartTestProcess(int test)
   this->EraseTest(test);
   this->RunningCount += GetProcessorsUsed(test);
 
-  cmCTestRunTest* testRun = new cmCTestRunTest(*this);
+  auto testRun = cm::make_unique<cmCTestRunTest>(*this);
+
   if (this->RepeatMode != cmCTest::Repeat::Never) {
     testRun->SetRepeatMode(this->RepeatMode);
     testRun->SetNumberOfRuns(this->RepeatCount);
@@ -229,28 +230,25 @@ bool cmCTestMultiProcessHandler::StartTestProcess(int test)
       e << "\n";
     }
     e << "Resource spec file:\n\n  " << this->TestHandler->ResourceSpecFile;
-    testRun->StartFailure(e.str(), "Insufficient resources");
-    this->FinishTestProcess(testRun, false);
+    cmCTestRunTest::StartFailure(std::move(testRun), e.str(),
+                                 "Insufficient resources");
     return false;
   }
 
   cmWorkingDirectory workdir(this->Properties[test]->Directory);
   if (workdir.Failed()) {
-    testRun->StartFailure("Failed to change working directory to " +
-                            this->Properties[test]->Directory + " : " +
-                            std::strerror(workdir.GetLastResult()),
-                          "Failed to change working directory");
-  } else {
-    if (testRun->StartTest(this->Completed, this->Total)) {
-      // Ownership of 'testRun' has moved to another structure.
-      // When the test finishes, FinishTestProcess will be called.
-      return true;
-    }
+    cmCTestRunTest::StartFailure(std::move(testRun),
+                                 "Failed to change working directory to " +
+                                   this->Properties[test]->Directory + " : " +
+                                   std::strerror(workdir.GetLastResult()),
+                                 "Failed to change working directory");
+    return false;
   }
 
-  // Pass ownership of 'testRun'.
-  this->FinishTestProcess(testRun, false);
-  return false;
+  // Ownership of 'testRun' has moved to another structure.
+  // When the test finishes, FinishTestProcess will be called.
+  return cmCTestRunTest::StartTest(std::move(testRun), this->Completed,
+                                   this->Total);
 }
 
 bool cmCTestMultiProcessHandler::AllocateResources(int index)
@@ -540,7 +538,8 @@ void cmCTestMultiProcessHandler::StartNextTests()
     if (this->SerialTestRunning) {
       break;
     }
-    // We can only start a RUN_SERIAL test if no other tests are also running.
+    // We can only start a RUN_SERIAL test if no other tests are also
+    // running.
     if (this->Properties[test]->RunSerial && this->RunningCount > 0) {
       continue;
     }
@@ -618,8 +617,8 @@ void cmCTestMultiProcessHandler::OnTestLoadRetryCB(uv_timer_t* timer)
   self->StartNextTests();
 }
 
-void cmCTestMultiProcessHandler::FinishTestProcess(cmCTestRunTest* runner,
-                                                   bool started)
+void cmCTestMultiProcessHandler::FinishTestProcess(
+  std::unique_ptr<cmCTestRunTest> runner, bool started)
 {
   this->Completed++;
 
@@ -631,7 +630,8 @@ void cmCTestMultiProcessHandler::FinishTestProcess(cmCTestRunTest* runner,
     this->SetStopTimePassed();
   }
   if (started) {
-    if (!this->StopTimePassed && runner->StartAgain(this->Completed)) {
+    if (!this->StopTimePassed &&
+        cmCTestRunTest::StartAgain(std::move(runner), this->Completed)) {
       this->Completed--; // remove the completed test because run again
       return;
     }
@@ -659,7 +659,7 @@ void cmCTestMultiProcessHandler::FinishTestProcess(cmCTestRunTest* runner,
   }
   properties->Affinity.clear();
 
-  delete runner;
+  runner.reset();
   if (started) {
     this->StartNextTests();
   }
diff --git a/Source/CTest/cmCTestMultiProcessHandler.h b/Source/CTest/cmCTestMultiProcessHandler.h
index 5b429d4ae8..c3686bc81f 100644
--- a/Source/CTest/cmCTestMultiProcessHandler.h
+++ b/Source/CTest/cmCTestMultiProcessHandler.h
@@ -6,6 +6,7 @@
 #include "cmConfigure.h" // IWYU pragma: keep
 
 #include <map>
+#include <memory>
 #include <set>
 #include <string>
 #include <vector>
@@ -124,7 +125,7 @@ protected:
   // Removes the checkpoint file
   void MarkFinished();
   void EraseTest(int index);
-  void FinishTestProcess(cmCTestRunTest* runner, bool started);
+  void FinishTestProcess(std::unique_ptr<cmCTestRunTest> runner, bool started);
 
   static void OnTestLoadRetryCB(uv_timer_t* timer);
 
diff --git a/Source/CTest/cmCTestRunTest.cxx b/Source/CTest/cmCTestRunTest.cxx
index ec549606fd..7d0f69bf5a 100644
--- a/Source/CTest/cmCTestRunTest.cxx
+++ b/Source/CTest/cmCTestRunTest.cxx
@@ -314,23 +314,27 @@ bool cmCTestRunTest::EndTest(size_t completed, size_t total, bool started)
   return passed || skipped;
 }
 
-bool cmCTestRunTest::StartAgain(size_t completed)
+bool cmCTestRunTest::StartAgain(std::unique_ptr<cmCTestRunTest> runner,
+                                size_t completed)
 {
-  if (!this->RunAgain) {
+  auto* testRun = runner.get();
+
+  if (!testRun->RunAgain) {
     return false;
   }
-  this->RunAgain = false; // reset
+  testRun->RunAgain = false; // reset
+  testRun->TestProcess = cm::make_unique<cmProcess>(std::move(runner));
   // change to tests directory
-  cmWorkingDirectory workdir(this->TestProperties->Directory);
+  cmWorkingDirectory workdir(testRun->TestProperties->Directory);
   if (workdir.Failed()) {
-    this->StartFailure("Failed to change working directory to " +
-                         this->TestProperties->Directory + " : " +
-                         std::strerror(workdir.GetLastResult()),
-                       "Failed to change working directory");
+    testRun->StartFailure("Failed to change working directory to " +
+                            testRun->TestProperties->Directory + " : " +
+                            std::strerror(workdir.GetLastResult()),
+                          "Failed to change working directory");
     return true;
   }
 
-  this->StartTest(completed, this->TotalNumberOfTests);
+  testRun->StartTest(completed, testRun->TotalNumberOfTests);
   return true;
 }
 
@@ -382,6 +386,18 @@ void cmCTestRunTest::MemCheckPostProcess()
   handler->PostProcessTest(this->TestResult, this->Index);
 }
 
+void cmCTestRunTest::StartFailure(std::unique_ptr<cmCTestRunTest> runner,
+                                  std::string const& output,
+                                  std::string const& detail)
+{
+  auto* testRun = runner.get();
+
+  testRun->TestProcess = cm::make_unique<cmProcess>(std::move(runner));
+  testRun->StartFailure(output, detail);
+
+  testRun->FinalizeTest(false);
+}
+
 void cmCTestRunTest::StartFailure(std::string const& output,
                                   std::string const& detail)
 {
@@ -413,7 +429,6 @@ void cmCTestRunTest::StartFailure(std::string const& output,
   this->TestResult.Path = this->TestProperties->Directory;
   this->TestResult.Output = output;
   this->TestResult.FullCommandLine.clear();
-  this->TestProcess = cm::make_unique<cmProcess>(*this);
 }
 
 std::string cmCTestRunTest::GetTestPrefix(size_t completed, size_t total) const
@@ -437,6 +452,21 @@ std::string cmCTestRunTest::GetTestPrefix(size_t completed, size_t total) const
   return outputStream.str();
 }
 
+bool cmCTestRunTest::StartTest(std::unique_ptr<cmCTestRunTest> runner,
+                               size_t completed, size_t total)
+{
+  auto* testRun = runner.get();
+
+  testRun->TestProcess = cm::make_unique<cmProcess>(std::move(runner));
+
+  if (!testRun->StartTest(completed, total)) {
+    testRun->FinalizeTest(false);
+    return false;
+  }
+
+  return true;
+}
+
 // Starts the execution of a test.  Returns once it has started
 bool cmCTestRunTest::StartTest(size_t completed, size_t total)
 {
@@ -468,7 +498,6 @@ bool cmCTestRunTest::StartTest(size_t completed, size_t total)
   if (this->TestProperties->Disabled) {
     this->TestResult.CompletionStatus = "Disabled";
     this->TestResult.Status = cmCTestTestHandler::NOT_RUN;
-    this->TestProcess = cm::make_unique<cmProcess>(*this);
     this->TestResult.Output = "Disabled";
     this->TestResult.FullCommandLine.clear();
     return false;
@@ -482,7 +511,6 @@ bool cmCTestRunTest::StartTest(size_t completed, size_t total)
   // its arguments are irrelevant. This matters for the case where a fixture
   // dependency might be creating the executable we want to run.
   if (!this->FailedDependencies.empty()) {
-    this->TestProcess = cm::make_unique<cmProcess>(*this);
     std::string msg = "Failed test dependencies:";
     for (std::string const& failedDep : this->FailedDependencies) {
       msg += " " + failedDep;
@@ -499,7 +527,6 @@ bool cmCTestRunTest::StartTest(size_t completed, size_t total)
   this->ComputeArguments();
   std::vector<std::string>& args = this->TestProperties->Args;
   if (args.size() >= 2 && args[1] == "NOT_AVAILABLE") {
-    this->TestProcess = cm::make_unique<cmProcess>(*this);
     std::string msg;
     if (this->CTest->GetConfigType().empty()) {
       msg = "Test not available without configuration.  (Missing \"-C "
@@ -521,7 +548,6 @@ bool cmCTestRunTest::StartTest(size_t completed, size_t total)
   for (std::string const& file : this->TestProperties->RequiredFiles) {
     if (!cmSystemTools::FileExists(file)) {
       // Required file was not found
-      this->TestProcess = cm::make_unique<cmProcess>(*this);
       *this->TestHandler->LogFile << "Unable to find required file: " << file
                                   << std::endl;
       cmCTestLog(this->CTest, ERROR_MESSAGE,
@@ -537,7 +563,6 @@ bool cmCTestRunTest::StartTest(size_t completed, size_t total)
   if (this->ActualCommand.empty()) {
     // if the command was not found create a TestResult object
     // that has that information
-    this->TestProcess = cm::make_unique<cmProcess>(*this);
     *this->TestHandler->LogFile << "Unable to find executable: " << args[1]
                                 << std::endl;
     cmCTestLog(this->CTest, ERROR_MESSAGE,
@@ -649,7 +674,6 @@ bool cmCTestRunTest::ForkProcess(cmDuration testTimeOut, bool explicitTimeout,
                                  std::vector<std::string>* environment,
                                  std::vector<size_t>* affinity)
 {
-  this->TestProcess = cm::make_unique<cmProcess>(*this);
   this->TestProcess->SetId(this->Index);
   this->TestProcess->SetWorkingDirectory(this->TestProperties->Directory);
   this->TestProcess->SetCommand(this->ActualCommand);
@@ -816,7 +840,8 @@ void cmCTestRunTest::WriteLogOutputTop(size_t completed, size_t total)
              "Testing " << this->TestProperties->Name << " ... ");
 }
 
-void cmCTestRunTest::FinalizeTest()
+void cmCTestRunTest::FinalizeTest(bool started)
 {
-  this->MultiTestHandler.FinishTestProcess(this, true);
+  this->MultiTestHandler.FinishTestProcess(this->TestProcess->GetRunner(),
+                                           started);
 }
diff --git a/Source/CTest/cmCTestRunTest.h b/Source/CTest/cmCTestRunTest.h
index 4988839b0f..b1d188ac21 100644
--- a/Source/CTest/cmCTestRunTest.h
+++ b/Source/CTest/cmCTestRunTest.h
@@ -65,6 +65,15 @@ public:
   // Read and store output.  Returns true if it must be called again.
   void CheckOutput(std::string const& line);
 
+  static bool StartTest(std::unique_ptr<cmCTestRunTest> runner,
+                        size_t completed, size_t total);
+  static bool StartAgain(std::unique_ptr<cmCTestRunTest> runner,
+                         size_t completed);
+
+  static void StartFailure(std::unique_ptr<cmCTestRunTest> runner,
+                           std::string const& output,
+                           std::string const& detail);
+
   // launch the test process, return whether it started correctly
   bool StartTest(size_t completed, size_t total);
   // capture and report the test results
@@ -74,8 +83,6 @@ public:
 
   void ComputeWeightedCost();
 
-  bool StartAgain(size_t completed);
-
   void StartFailure(std::string const& output, std::string const& detail);
 
   cmCTest* GetCTest() const { return this->CTest; }
@@ -84,7 +91,7 @@ public:
 
   const std::vector<std::string>& GetArguments() { return this->Arguments; }
 
-  void FinalizeTest();
+  void FinalizeTest(bool started = true);
 
   bool TimedOutForStopTime() const { return this->TimeoutIsForStopTime; }
 
diff --git a/Source/CTest/cmProcess.cxx b/Source/CTest/cmProcess.cxx
index cdf899c497..76ffb20a74 100644
--- a/Source/CTest/cmProcess.cxx
+++ b/Source/CTest/cmProcess.cxx
@@ -5,6 +5,7 @@
 #include <csignal>
 #include <iostream>
 #include <string>
+#include <utility>
 
 #include <cmext/algorithm>
 
@@ -18,12 +19,11 @@
 #if defined(_WIN32)
 #  include "cm_kwiml.h"
 #endif
-#include <utility>
 
 #define CM_PROCESS_BUF_SIZE 65536
 
-cmProcess::cmProcess(cmCTestRunTest& runner)
-  : Runner(runner)
+cmProcess::cmProcess(std::unique_ptr<cmCTestRunTest> runner)
+  : Runner(std::move(runner))
   , Conv(cmProcessOutput::UTF8, CM_PROCESS_BUF_SIZE)
 {
   this->Timeout = cmDuration::zero();
@@ -69,7 +69,7 @@ bool cmProcess::StartProcess(uv_loop_t& loop, std::vector<size_t>* affinity)
   cm::uv_timer_ptr timer;
   int status = timer.init(loop, this);
   if (status != 0) {
-    cmCTestLog(this->Runner.GetCTest(), ERROR_MESSAGE,
+    cmCTestLog(this->Runner->GetCTest(), ERROR_MESSAGE,
                "Error initializing timer: " << uv_strerror(status)
                                             << std::endl);
     return false;
@@ -84,7 +84,7 @@ bool cmProcess::StartProcess(uv_loop_t& loop, std::vector<size_t>* affinity)
   int fds[2] = { -1, -1 };
   status = cmGetPipes(fds);
   if (status != 0) {
-    cmCTestLog(this->Runner.GetCTest(), ERROR_MESSAGE,
+    cmCTestLog(this->Runner->GetCTest(), ERROR_MESSAGE,
                "Error initializing pipe: " << uv_strerror(status)
                                            << std::endl);
     return false;
@@ -127,7 +127,7 @@ bool cmProcess::StartProcess(uv_loop_t& loop, std::vector<size_t>* affinity)
     uv_read_start(pipe_reader, &cmProcess::OnAllocateCB, &cmProcess::OnReadCB);
 
   if (status != 0) {
-    cmCTestLog(this->Runner.GetCTest(), ERROR_MESSAGE,
+    cmCTestLog(this->Runner->GetCTest(), ERROR_MESSAGE,
                "Error starting read events: " << uv_strerror(status)
                                               << std::endl);
     return false;
@@ -135,7 +135,7 @@ bool cmProcess::StartProcess(uv_loop_t& loop, std::vector<size_t>* affinity)
 
   status = this->Process.spawn(loop, options, this);
   if (status != 0) {
-    cmCTestLog(this->Runner.GetCTest(), ERROR_MESSAGE,
+    cmCTestLog(this->Runner->GetCTest(), ERROR_MESSAGE,
                "Process not started\n " << this->Command << "\n["
                                         << uv_strerror(status) << "]\n");
     return false;
@@ -152,7 +152,7 @@ bool cmProcess::StartProcess(uv_loop_t& loop, std::vector<size_t>* affinity)
 
 void cmProcess::StartTimer()
 {
-  auto properties = this->Runner.GetTestProperties();
+  auto properties = this->Runner->GetTestProperties();
   auto msec =
     std::chrono::duration_cast<std::chrono::milliseconds>(this->Timeout);
 
@@ -222,7 +222,7 @@ void cmProcess::OnRead(ssize_t nread, const uv_buf_t* buf)
     cm::append(this->Output, strdata);
 
     while (this->Output.GetLine(line)) {
-      this->Runner.CheckOutput(line);
+      this->Runner->CheckOutput(line);
       line.clear();
     }
 
@@ -236,20 +236,20 @@ void cmProcess::OnRead(ssize_t nread, const uv_buf_t* buf)
   // The process will provide no more data.
   if (nread != UV_EOF) {
     auto error = static_cast<int>(nread);
-    cmCTestLog(this->Runner.GetCTest(), ERROR_MESSAGE,
+    cmCTestLog(this->Runner->GetCTest(), ERROR_MESSAGE,
                "Error reading stream: " << uv_strerror(error) << std::endl);
   }
 
   // Look for partial last lines.
   if (this->Output.GetLast(line)) {
-    this->Runner.CheckOutput(line);
+    this->Runner->CheckOutput(line);
   }
 
   this->ReadHandleClosed = true;
   this->PipeReader.reset();
   if (this->ProcessHandleClosed) {
     uv_timer_stop(this->Timer);
-    this->Runner.FinalizeTest();
+    this->Runner->FinalizeTest();
   }
 }
 
@@ -291,7 +291,7 @@ void cmProcess::OnTimeout()
     // Our on-exit handler already ran but did not finish the test
     // because we were still reading output.  We've just dropped
     // our read handler, so we need to finish the test now.
-    this->Runner.FinalizeTest();
+    this->Runner->FinalizeTest();
   }
 }
 
@@ -333,7 +333,7 @@ void cmProcess::OnExit(int64_t exit_status, int term_signal)
   this->ProcessHandleClosed = true;
   if (this->ReadHandleClosed) {
     uv_timer_stop(this->Timer);
-    this->Runner.FinalizeTest();
+    this->Runner->FinalizeTest();
   }
 }
 
diff --git a/Source/CTest/cmProcess.h b/Source/CTest/cmProcess.h
index 2c24f2df02..0f69f68c8b 100644
--- a/Source/CTest/cmProcess.h
+++ b/Source/CTest/cmProcess.h
@@ -6,7 +6,9 @@
 #include "cmConfigure.h" // IWYU pragma: keep
 
 #include <chrono>
+#include <memory>
 #include <string>
+#include <utility>
 #include <vector>
 
 #include <stddef.h>
@@ -28,7 +30,7 @@ class cmCTestRunTest;
 class cmProcess
 {
 public:
-  explicit cmProcess(cmCTestRunTest& runner);
+  explicit cmProcess(std::unique_ptr<cmCTestRunTest> runner);
   ~cmProcess();
   void SetCommand(std::string const& command);
   void SetCommandArguments(std::vector<std::string> const& arg);
@@ -70,6 +72,11 @@ public:
   Exception GetExitException();
   std::string GetExitExceptionString();
 
+  std::unique_ptr<cmCTestRunTest> GetRunner()
+  {
+    return std::move(this->Runner);
+  }
+
 private:
   cmDuration Timeout;
   std::chrono::steady_clock::time_point StartTime;
@@ -82,7 +89,7 @@ private:
   cm::uv_timer_ptr Timer;
   std::vector<char> Buf;
 
-  cmCTestRunTest& Runner;
+  std::unique_ptr<cmCTestRunTest> Runner;
   cmProcessOutput Conv;
   int Signal = 0;
   cmProcess::State ProcessState = cmProcess::State::Starting;