Merge topic 'modernize-memory-management'

44867a8c01 Modernize memory management

Acked-by: Kitware Robot <[email protected]>
Merge-request: !4444

Source/CPack/OSXScriptLauncher.cxx

@@ -5,6 +5,8 @@
 #include <string>
 #include <vector>
 
+#include <cm/memory>
+
 #include <CoreFoundation/CoreFoundation.h>
 
 #include "cmsys/FStream.hxx"
@@ -26,7 +28,6 @@ int main(int argc, char* argv[])
   CFStringRef fileName;
   CFBundleRef appBundle;
   CFURLRef scriptFileURL;
-  UInt8* path;
 
   // get CF URL for script
   if (!(appBundle = CFBundleGetMainBundle())) {
@@ -41,13 +42,15 @@ int main(int argc, char* argv[])
   }
 
   // create path string
-  if (!(path = new UInt8[PATH_MAX])) {
+  auto path = cm::make_unique<UInt8[]>(PATH_MAX);
+  if (!path) {
     return 1;
   }
 
   // get the file system path of the url as a cstring
   // in an encoding suitable for posix apis
-  if (!CFURLGetFileSystemRepresentation(scriptFileURL, true, path, PATH_MAX)) {
+  if (!CFURLGetFileSystemRepresentation(scriptFileURL, true, path.get(),
+                                        PATH_MAX)) {
     DebugError("CFURLGetFileSystemRepresentation failed");
     return 1;
   }
@@ -55,10 +58,10 @@ int main(int argc, char* argv[])
   // dispose of the CF variable
   CFRelease(scriptFileURL);
 
-  std::string fullScriptPath = reinterpret_cast<char*>(path);
-  delete[] path;
+  std::string fullScriptPath = reinterpret_cast<char*>(path.get());
+  path.reset();
 
-  if (!cmsys::SystemTools::FileExists(fullScriptPath.c_str())) {
+  if (!cmsys::SystemTools::FileExists(fullScriptPath)) {
     return 1;
   }
 
@@ -80,7 +83,6 @@ int main(int argc, char* argv[])
   cmsysProcess_SetTimeout(cp, 0);
   cmsysProcess_Execute(cp);
 
-  std::vector<char> tempOutput;
   char* data;
   int length;
   while (cmsysProcess_WaitForData(cp, &data, &length, nullptr)) {

Source/CPack/WiX/cmCPackWIXGenerator.cxx

@@ -4,6 +4,7 @@
 
 #include <algorithm>
 
+#include <cm/memory>
 #include <cm/string_view>
 
 #include "cmsys/Directory.hxx"
@@ -35,22 +36,16 @@
 #include "cmCMakeToWixPath.h"
 
 cmCPackWIXGenerator::cmCPackWIXGenerator()
-  : Patch(0)
-  , ComponentGuidType(cmWIXSourceWriter::WIX_GENERATED_GUID)
+  : ComponentGuidType(cmWIXSourceWriter::WIX_GENERATED_GUID)
 {
 }
 
-cmCPackWIXGenerator::~cmCPackWIXGenerator()
-{
-  if (this->Patch) {
-    delete this->Patch;
-  }
-}
+cmCPackWIXGenerator::~cmCPackWIXGenerator() = default;
 
 int cmCPackWIXGenerator::InitializeInternal()
 {
   componentPackageMethod = ONE_PACKAGE;
-  this->Patch = new cmWIXPatch(this->Logger);
+  this->Patch = cm::make_unique<cmWIXPatch>(this->Logger);
 
   return this->Superclass::InitializeInternal();
 }

Source/CPack/WiX/cmCPackWIXGenerator.h

@@ -4,6 +4,7 @@
 #define cmCPackWIXGenerator_h
 
 #include <map>
+#include <memory>
 #include <string>
 
 #include "cmCPackGenerator.h"
@@ -24,6 +25,8 @@ public:
   cmCPackTypeMacro(cmCPackWIXGenerator, cmCPackGenerator);
 
   cmCPackWIXGenerator();
+  cmCPackWIXGenerator(const cmCPackWIXGenerator&) = delete;
+  const cmCPackWIXGenerator& operator=(const cmCPackWIXGenerator&) = delete;
   ~cmCPackWIXGenerator();
 
 protected:
@@ -157,7 +160,7 @@ private:
 
   std::string CPackTopLevel;
 
-  cmWIXPatch* Patch;
+  std::unique_ptr<cmWIXPatch> Patch;
 
   cmWIXSourceWriter::GuidType ComponentGuidType;
 };

Source/CPack/WiX/cmWIXPatch.cxx

@@ -41,7 +41,7 @@ void cmWIXPatch::ApplyFragment(std::string const& id,
 void cmWIXPatch::ApplyElementChildren(const cmWIXPatchElement& element,
                                       cmWIXSourceWriter& writer)
 {
-  for (cmWIXPatchNode* node : element.children) {
+  for (const auto& node : element.children) {
     switch (node->type()) {
       case cmWIXPatchNode::ELEMENT:
         ApplyElement(dynamic_cast<const cmWIXPatchElement&>(*node), writer);

Source/CPack/WiX/cmWIXPatchParser.cxx

@@ -2,6 +2,10 @@
    file Copyright.txt or https://cmake.org/licensing for details.  */
 #include "cmWIXPatchParser.h"
 
+#include <utility>
+
+#include <cm/memory>
+
 #include "cm_expat.h"
 
 #include "cmCPackGenerator.h"
@@ -20,12 +24,8 @@ cmWIXPatchNode::~cmWIXPatchNode()
 {
 }
 
-cmWIXPatchElement::~cmWIXPatchElement()
-{
-  for (cmWIXPatchNode* child : children) {
-    delete child;
-  }
-}
+cmWIXPatchElement::cmWIXPatchElement() = default;
+cmWIXPatchElement::~cmWIXPatchElement() = default;
 
 cmWIXPatchParser::cmWIXPatchParser(fragment_map_t& fragments,
                                    cmCPackLog* logger)
@@ -54,8 +54,7 @@ void cmWIXPatchParser::StartElement(const std::string& name, const char** atts)
   } else if (State == INSIDE_FRAGMENT) {
     cmWIXPatchElement& parent = *ElementStack.back();
 
-    cmWIXPatchElement* element = new cmWIXPatchElement;
-    parent.children.push_back(element);
+    auto element = cm::make_unique<cmWIXPatchElement>();
 
     element->name = name;
 
@@ -66,7 +65,8 @@ void cmWIXPatchParser::StartElement(const std::string& name, const char** atts)
       element->attributes[key] = value;
     }
 
-    ElementStack.push_back(element);
+    ElementStack.push_back(element.get());
+    parent.children.push_back(std::move(element));
   }
 }
 
@@ -130,10 +130,10 @@ void cmWIXPatchParser::CharacterDataHandler(const char* data, int length)
     std::string::size_type last = text.find_last_not_of(whitespace);
 
     if (first != std::string::npos && last != std::string::npos) {
-      cmWIXPatchText* text_node = new cmWIXPatchText;
+      auto text_node = cm::make_unique<cmWIXPatchText>();
       text_node->text = text.substr(first, last - first + 1);
 
-      parent.children.push_back(text_node);
+      parent.children.push_back(std::move(text_node));
     }
   }
 }

Source/CPack/WiX/cmWIXPatchParser.h

@@ -4,6 +4,7 @@
 #define cmCPackWIXPatchParser_h
 
 #include <map>
+#include <memory>
 #include <vector>
 
 #include "cmCPackLog.h"
@@ -33,9 +34,14 @@ struct cmWIXPatchElement : cmWIXPatchNode
 {
   virtual Type type();
 
+  cmWIXPatchElement();
+
+  cmWIXPatchElement(const cmWIXPatchElement&) = delete;
+  const cmWIXPatchElement& operator=(const cmWIXPatchElement&) = delete;
+
   ~cmWIXPatchElement();
 
-  using child_list_t = std::vector<cmWIXPatchNode*>;
+  using child_list_t = std::vector<std::unique_ptr<cmWIXPatchNode>>;
   using attributes_t = std::map<std::string, std::string>;
 
   std::string name;

Source/CPack/cmCPackLog.cxx

@@ -4,54 +4,38 @@
 
 #include <iostream>
 
+#include <cm/memory>
+
 #include "cmGeneratedFileStream.h"
 #include "cmSystemTools.h"
 
 cmCPackLog::cmCPackLog()
 {
-  this->Verbose = false;
-  this->Debug = false;
-  this->Quiet = false;
-  this->NewLine = true;
-
-  this->LastTag = cmCPackLog::NOTAG;
   this->DefaultOutput = &std::cout;
   this->DefaultError = &std::cerr;
-
-  this->LogOutput = nullptr;
-  this->LogOutputCleanup = false;
 }
 
-cmCPackLog::~cmCPackLog()
-{
-  this->SetLogOutputStream(nullptr);
-}
+cmCPackLog::~cmCPackLog() = default;
 
 void cmCPackLog::SetLogOutputStream(std::ostream* os)
 {
-  if (this->LogOutputCleanup && this->LogOutput) {
-    delete this->LogOutput;
-  }
-  this->LogOutputCleanup = false;
+  this->LogOutputStream.reset();
   this->LogOutput = os;
 }
 
 bool cmCPackLog::SetLogOutputFile(const char* fname)
 {
-  cmGeneratedFileStream* cg = nullptr;
+  this->LogOutputStream.reset();
   if (fname) {
-    cg = new cmGeneratedFileStream(fname);
-  }
-  if (cg && !*cg) {
-    delete cg;
-    cg = nullptr;
+    this->LogOutputStream = cm::make_unique<cmGeneratedFileStream>(fname);
   }
-  this->SetLogOutputStream(cg);
-  if (!cg) {
-    return false;
+  if (this->LogOutputStream && !*this->LogOutputStream) {
+    this->LogOutputStream.reset();
   }
-  this->LogOutputCleanup = true;
-  return true;
+
+  this->LogOutput = this->LogOutputStream.get();
+
+  return this->LogOutput != nullptr;
 }
 
 void cmCPackLog::Log(int tag, const char* file, int line, const char* msg,

Source/CPack/cmCPackLog.h

@@ -5,6 +5,7 @@
 
 #include "cmConfigure.h" // IWYU pragma: keep
 
+#include <memory>
 #include <ostream>
 #include <string>
 
@@ -97,13 +98,13 @@ public:
   void SetErrorPrefix(std::string const& pfx) { this->ErrorPrefix = pfx; }
 
 private:
-  bool Verbose;
-  bool Debug;
-  bool Quiet;
+  bool Verbose = false;
+  bool Debug = false;
+  bool Quiet = false;
 
-  bool NewLine;
+  bool NewLine = true;
 
-  int LastTag;
+  int LastTag = cmCPackLog::NOTAG;
 
   std::string Prefix;
   std::string OutputPrefix;
@@ -112,13 +113,11 @@ private:
   std::string WarningPrefix;
   std::string ErrorPrefix;
 
-  std::ostream* DefaultOutput;
-  std::ostream* DefaultError;
+  std::ostream* DefaultOutput = nullptr;
+  std::ostream* DefaultError = nullptr;
 
-  std::string LogOutputFileName;
-  std::ostream* LogOutput;
-  // Do we need to cleanup log output stream
-  bool LogOutputCleanup;
+  std::ostream* LogOutput = nullptr;
+  std::unique_ptr<std::ostream> LogOutputStream;
 };
 
 class cmCPackLogWrite

Source/CTest/cmCTestScriptHandler.cxx

@@ -6,7 +6,6 @@
 #include <cstdlib>
 #include <cstring>
 #include <map>
-#include <memory>
 #include <ratio>
 #include <sstream>
 #include <utility>
@@ -51,22 +50,7 @@
 
 #define CTEST_INITIAL_CMAKE_OUTPUT_FILE_NAME "CTestInitialCMakeOutput.log"
 
-cmCTestScriptHandler::cmCTestScriptHandler()
-{
-  this->Backup = false;
-  this->EmptyBinDir = false;
-  this->EmptyBinDirOnce = false;
-  this->Makefile = nullptr;
-  this->ParentMakefile = nullptr;
-  this->CMake = nullptr;
-  this->GlobalGenerator = nullptr;
-
-  this->ScriptStartTime = std::chrono::steady_clock::time_point();
-
-  // the *60 is because the settings are in minutes but GetTime is seconds
-  this->MinimumInterval = 30 * 60;
-  this->ContinuousDuration = -1;
-}
+cmCTestScriptHandler::cmCTestScriptHandler() = default;
 
 void cmCTestScriptHandler::Initialize()
 {
@@ -95,22 +79,15 @@ void cmCTestScriptHandler::Initialize()
   // what time in seconds did this script start running
   this->ScriptStartTime = std::chrono::steady_clock::time_point();
 
-  delete this->Makefile;
-  this->Makefile = nullptr;
+  this->Makefile.reset();
   this->ParentMakefile = nullptr;
 
-  delete this->GlobalGenerator;
-  this->GlobalGenerator = nullptr;
+  this->GlobalGenerator.reset();
 
-  delete this->CMake;
+  this->CMake.reset();
 }
 
-cmCTestScriptHandler::~cmCTestScriptHandler()
-{
-  delete this->Makefile;
-  delete this->GlobalGenerator;
-  delete this->CMake;
-}
+cmCTestScriptHandler::~cmCTestScriptHandler() = default;
 
 // just adds an argument to the vector
 void cmCTestScriptHandler::AddConfigurationScript(const char* script,
@@ -247,23 +224,20 @@ int cmCTestScriptHandler::ExecuteScript(const std::string& total_script_arg)
 void cmCTestScriptHandler::CreateCMake()
 {
   // create a cmake instance to read the configuration script
-  if (this->CMake) {
-    delete this->CMake;
-    delete this->GlobalGenerator;
-    delete this->Makefile;
-  }
-  this->CMake = new cmake(cmake::RoleScript, cmState::CTest);
+  this->CMake = cm::make_unique<cmake>(cmake::RoleScript, cmState::CTest);
   this->CMake->SetHomeDirectory("");
   this->CMake->SetHomeOutputDirectory("");
   this->CMake->GetCurrentSnapshot().SetDefaultDefinitions();
   this->CMake->AddCMakePaths();
-  this->GlobalGenerator = new cmGlobalGenerator(this->CMake);
+  this->GlobalGenerator =
+    cm::make_unique<cmGlobalGenerator>(this->CMake.get());
 
   cmStateSnapshot snapshot = this->CMake->GetCurrentSnapshot();
   std::string cwd = cmSystemTools::GetCurrentWorkingDirectory();
   snapshot.GetDirectory().SetCurrentSource(cwd);
   snapshot.GetDirectory().SetCurrentBinary(cwd);
-  this->Makefile = new cmMakefile(this->GlobalGenerator, snapshot);
+  this->Makefile =
+    cm::make_unique<cmMakefile>(this->GlobalGenerator.get(), snapshot);
   if (this->ParentMakefile) {
     this->Makefile->SetRecursionDepth(
       this->ParentMakefile->GetRecursionDepth());
@@ -878,7 +852,7 @@ bool cmCTestScriptHandler::RunScript(cmCTest* ctest, cmMakefile* mf,
                                      const char* sname, bool InProcess,
                                      int* returnValue)
 {
-  cmCTestScriptHandler* sh = new cmCTestScriptHandler();
+  auto sh = cm::make_unique<cmCTestScriptHandler>();
   sh->SetCTestInstance(ctest);
   sh->ParentMakefile = mf;
   sh->AddConfigurationScript(sname, InProcess);
@@ -886,7 +860,6 @@ bool cmCTestScriptHandler::RunScript(cmCTest* ctest, cmMakefile* mf,
   if (returnValue) {
     *returnValue = res;
   }
-  delete sh;
   return true;
 }
 

Source/CTest/cmCTestScriptHandler.h

@@ -101,12 +101,14 @@ public:
   cmDuration GetRemainingTimeAllowed();
 
   cmCTestScriptHandler();
+  cmCTestScriptHandler(const cmCTestScriptHandler&) = delete;
+  const cmCTestScriptHandler& operator=(const cmCTestScriptHandler&) = delete;
   ~cmCTestScriptHandler() override;
 
   void Initialize() override;
 
   void CreateCMake();
-  cmake* GetCMake() { return this->CMake; }
+  cmake* GetCMake() { return this->CMake.get(); }
 
   void SetRunCurrentScript(bool value);
 
@@ -143,9 +145,9 @@ private:
 
   bool ShouldRunCurrentScript;
 
-  bool Backup;
-  bool EmptyBinDir;
-  bool EmptyBinDirOnce;
+  bool Backup = false;
+  bool EmptyBinDir = false;
+  bool EmptyBinDirOnce = false;
 
   std::string SourceDir;
   std::string BinaryDir;
@@ -161,16 +163,18 @@ private:
   std::string CMOutFile;
   std::vector<std::string> ExtraUpdates;
 
-  double MinimumInterval;
-  double ContinuousDuration;
+  // the *60 is because the settings are in minutes but GetTime is seconds
+  double MinimumInterval = 30 * 60;
+  double ContinuousDuration = -1;
 
   // what time in seconds did this script start running
-  std::chrono::steady_clock::time_point ScriptStartTime;
+  std::chrono::steady_clock::time_point ScriptStartTime =
+    std::chrono::steady_clock::time_point();
 
-  cmMakefile* Makefile;
-  cmMakefile* ParentMakefile;
-  cmGlobalGenerator* GlobalGenerator;
-  cmake* CMake;
+  std::unique_ptr<cmMakefile> Makefile;
+  cmMakefile* ParentMakefile = nullptr;
+  std::unique_ptr<cmGlobalGenerator> GlobalGenerator;
+  std::unique_ptr<cmake> CMake;
 };
 
 #endif

Source/CTest/cmCTestTestHandler.cxx

@@ -1267,7 +1267,7 @@ void cmCTestTestHandler::ProcessDirectory(std::vector<std::string>& passed,
   this->StartTestTime = std::chrono::system_clock::now();
   auto elapsed_time_start = std::chrono::steady_clock::now();
 
-  cmCTestMultiProcessHandler* parallel = new cmCTestMultiProcessHandler;
+  auto parallel = cm::make_unique<cmCTestMultiProcessHandler>();
   parallel->SetCTest(this->CTest);
   parallel->SetParallelLevel(this->CTest->GetParallelLevel());
   parallel->SetTestHandler(this);
@@ -1338,7 +1338,6 @@ void cmCTestTestHandler::ProcessDirectory(std::vector<std::string>& passed,
   } else {
     parallel->RunTests();
   }
-  delete parallel;
   this->EndTest = this->CTest->CurrentTime();
   this->EndTestTime = std::chrono::system_clock::now();
   this->ElapsedTestingTime =
@@ -2016,13 +2015,13 @@ void cmCTestTestHandler::GenerateRegressionImages(cmXMLWriter& xml,
                                 | std::ios::binary
 #endif
           );
-          unsigned char* file_buffer = new unsigned char[len + 1];
-          ifs.read(reinterpret_cast<char*>(file_buffer), len);
-          unsigned char* encoded_buffer = new unsigned char[static_cast<int>(
-            static_cast<double>(len) * 1.5 + 5.0)];
+          auto file_buffer = cm::make_unique<unsigned char[]>(len + 1);
+          ifs.read(reinterpret_cast<char*>(file_buffer.get()), len);
+          auto encoded_buffer = cm::make_unique<unsigned char[]>(
+            static_cast<int>(static_cast<double>(len) * 1.5 + 5.0));
 
-          size_t rlen =
-            cmsysBase64_Encode(file_buffer, len, encoded_buffer, 1);
+          size_t rlen = cmsysBase64_Encode(file_buffer.get(), len,
+                                           encoded_buffer.get(), 1);
 
           xml.StartElement("NamedMeasurement");
           xml.Attribute(measurementfile.match(1).c_str(),
@@ -2039,8 +2038,6 @@ void cmCTestTestHandler::GenerateRegressionImages(cmXMLWriter& xml,
           }
           xml.Element("Value", ostr.str());
           xml.EndElement(); // NamedMeasurement
-          delete[] file_buffer;
-          delete[] encoded_buffer;
         }
       } else {
         int idx = 4;
@@ -2085,11 +2082,10 @@ void cmCTestTestHandler::SetTestsToRunInformation(const char* in)
   if (cmSystemTools::FileExists(in)) {
     cmsys::ifstream fin(in);
     unsigned long filelen = cmSystemTools::FileLength(in);
-    char* buff = new char[filelen + 1];
-    fin.getline(buff, filelen);
+    auto buff = cm::make_unique<char[]>(filelen + 1);
+    fin.getline(buff.get(), filelen);
     buff[fin.gcount()] = 0;
-    this->TestsToRunString = buff;
-    delete[] buff;
+    this->TestsToRunString = buff.get();
   }
 }
 

Source/CTest/cmParsePHPCoverage.cxx

@@ -3,6 +3,8 @@
 #include <cstdlib>
 #include <cstring>
 
+#include <cm/memory>
+
 #include "cmsys/Directory.hxx"
 #include "cmsys/FStream.hxx"
 
@@ -142,17 +144,15 @@ bool cmParsePHPCoverage::ReadFileInformation(std::istream& in)
   int size = 0;
   if (this->ReadInt(in, size)) {
     size++; // add one for null termination
-    char* s = new char[size + 1];
+    auto s = cm::make_unique<char[]>(size + 1);
     // read open quote
     if (in.get(c) && c != '"') {
-      delete[] s;
       return false;
     }
     // read the string data
-    in.read(s, size - 1);
+    in.read(s.get(), size - 1);
     s[size - 1] = 0;
-    std::string fileName = s;
-    delete[] s;
+    std::string fileName = s.get();
     // read close quote
     if (in.get(c) && c != '"') {
       cmCTestLog(this->CTest, ERROR_MESSAGE,