Swift: make TargetFile::good() a class invariant

Fallible initialization has been moved to a factory function, and
`commit` has been moved to the destructor.

Author: redsun82

swift/extractor/SwiftExtractor.cpp

@@ -77,6 +77,20 @@ static llvm::SmallVector<swift::Decl*> getTopLevelDecls(swift::ModuleDecl& modul
   return ret;
 }
 
+static void dumpArgs(TargetFile& out, const SwiftExtractorConfiguration& config) {
+  out << "/* extractor-args:\n";
+  for (const auto& opt : config.frontendOptions) {
+    out << "  " << std::quoted(opt) << " \\\n";
+  }
+  out << "\n*/\n";
+
+  out << "/* swift-frontend-args:\n";
+  for (const auto& opt : config.patchedFrontendOptions) {
+    out << "  " << std::quoted(opt) << " \\\n";
+  }
+  out << "\n*/\n";
+}
+
 static void extractDeclarations(const SwiftExtractorConfiguration& config,
                                 swift::CompilerInstance& compiler,
                                 swift::ModuleDecl& module,
@@ -86,25 +100,13 @@ static void extractDeclarations(const SwiftExtractorConfiguration& config,
   // The extractor can be called several times from different processes with
   // the same input file(s). Using `TargetFile` the first process will win, and the following
   // will just skip the work
-  TargetFile trapStream{filename + ".trap", config.trapDir, config.getTempTrapDir()};
-  if (!trapStream.good()) {
+  auto trapTarget = TargetFile::create(filename + ".trap", config.trapDir, config.getTempTrapDir());
+  if (!trapTarget) {
     // another process arrived first, nothing to do for us
     return;
   }
-
-  trapStream << "/* extractor-args:\n";
-  for (const auto& opt : config.frontendOptions) {
-    trapStream << "  " << std::quoted(opt) << " \\\n";
-  }
-  trapStream << "\n*/\n";
-
-  trapStream << "/* swift-frontend-args:\n";
-  for (const auto& opt : config.patchedFrontendOptions) {
-    trapStream << "  " << std::quoted(opt) << " \\\n";
-  }
-  trapStream << "\n*/\n";
-
-  TrapDomain trap{trapStream};
+  dumpArgs(*trapTarget, config);
+  TrapDomain trap{*trapTarget};
 
   // TODO: move default location emission elsewhere, possibly in a separate global trap file
   // the following cannot conflict with actual files as those have an absolute path starting with /
@@ -129,7 +131,6 @@ static void extractDeclarations(const SwiftExtractorConfiguration& config,
     auto fileLabel = trap.createLabel<FileTag>(name.str().str());
     trap.emit(FilesTrap{fileLabel, name.str().str()});
   }
-  trapStream.commit();
 }
 
 static std::unordered_set<std::string> collectInputFilenames(swift::CompilerInstance& compiler) {

swift/extractor/infra/TargetFile.cpp

@@ -1,5 +1,10 @@
 #include "swift/extractor/infra/TargetFile.h"
 
+#include <iostream>
+#include <cstdio>
+#include <cerrno>
+#include <system_error>
+
 #include <llvm/Support/FileSystem.h>
 #include <llvm/Support/Path.h>
 
@@ -36,31 +41,49 @@ std::string initPath(std::string_view target, std::string_view dir) {
 TargetFile::TargetFile(std::string_view target,
                        std::string_view targetDir,
                        std::string_view workingDir)
-    : workingPath{initPath(target, workingDir)}, targetPath{initPath(target, targetDir)} {
+    : workingPath{initPath(target, workingDir)}, targetPath{initPath(target, targetDir)} {}
+
+bool TargetFile::init() {
   errno = 0;
   // since C++17 "x" mode opens with O_EXCL (fails if file already exists)
   if (auto f = std::fopen(targetPath.c_str(), "wx")) {
     std::fclose(f);
     out.open(workingPath);
     checkOutput("open file for writing");
-  } else {
-    if (errno != EEXIST) {
-      error("open file for writing", targetPath);
-    }
-    // else we just lost the race, do nothing (good() will return false to signal this)
+    return true;
   }
+  if (errno != EEXIST) {
+    error("open file for writing", targetPath);
+  }
+  // else we just lost the race
+  return false;
+}
+
+std::optional<TargetFile> TargetFile::create(std::string_view target,
+                                             std::string_view targetDir,
+                                             std::string_view workingDir) {
+  TargetFile ret{target, targetDir, workingDir};
+  if (ret.init()) return {std::move(ret)};
+  return std::nullopt;
 }
 
-bool TargetFile::good() const {
-  return out && out.is_open();
+TargetFile& TargetFile::operator=(TargetFile&& other) {
+  if (this != &other) {
+    commit();
+    workingPath = std::move(other.workingPath);
+    targetPath = std::move(other.targetPath);
+    out = std::move(other.out);
+  }
+  return *this;
 }
 
 void TargetFile::commit() {
-  assert(good());
-  out.close();
-  errno = 0;
-  if (std::rename(workingPath.c_str(), targetPath.c_str()) != 0) {
-    error("rename file", targetPath);
+  if (out.is_open()) {
+    out.close();
+    errno = 0;
+    if (std::rename(workingPath.c_str(), targetPath.c_str()) != 0) {
+      error("rename file", targetPath);
+    }
   }
 }
 
@@ -69,5 +92,4 @@ void TargetFile::checkOutput(const char* action) {
     error(action, workingPath);
   }
 }
-
 }  // namespace codeql

swift/extractor/infra/TargetFile.h

@@ -2,28 +2,29 @@
 
 #include <string>
 #include <fstream>
-#include <iostream>
-#include <cstdio>
+#include <optional>
 #include <cerrno>
-#include <system_error>
-#include <sstream>
 
 namespace codeql {
 
-// Only the first process trying to open an `TargetFile` for a given `target` is allowed to do
-// so, all others will have an instance with `good() == false` and failing on any other operation.
+// Only the first process trying to create a `TargetFile` for a given `target` is allowed to do
+// so, all others will have `create` return `std::nullopt`.
 // The content streamed to the `TargetFile` is written to `workingDir/target`, and is moved onto
-// `targetDir/target` only when `commit()` is called
+// `targetDir/target` on destruction.
 class TargetFile {
   std::string workingPath;
   std::string targetPath;
   std::ofstream out;
 
  public:
-  TargetFile(std::string_view target, std::string_view targetDir, std::string_view workingDir);
+  static std::optional<TargetFile> create(std::string_view target,
+                                          std::string_view targetDir,
+                                          std::string_view workingDir);
 
-  bool good() const;
-  void commit();
+  ~TargetFile() { commit(); }
+
+  TargetFile(TargetFile&& other) = default;
+  TargetFile& operator=(TargetFile&& other);
 
   template <typename T>
   TargetFile& operator<<(T&& value) {
@@ -34,7 +35,11 @@ class TargetFile {
   }
 
  private:
+  TargetFile(std::string_view target, std::string_view targetDir, std::string_view workingDir);
+
+  bool init();
   void checkOutput(const char* action);
+  void commit();
 };
 
 }  // namespace codeql