Swift: move back file opening code

Author: redsun82

swift/extractor/SwiftExtractor.cpp

@@ -48,16 +48,53 @@ static void extractFile(const SwiftExtractorConfiguration& config, swift::Source
     return;
   }
 
-  if (TrapOutput trap{config, file.getFilename().str()}) {
-    TrapArena arena{};
+  // The extractor can be called several times from different processes with
+  // the same input file(s)
+  // We are using PID to avoid concurrent access
+  // TODO: find a more robust approach to avoid collisions?
+  std::string tempTrapName = file.getFilename().str() + '.' + std::to_string(getpid()) + ".trap";
+  llvm::SmallString<PATH_MAX> tempTrapPath(config.trapDir);
+  llvm::sys::path::append(tempTrapPath, tempTrapName);
 
-    auto label = arena.allocateLabel<File::Tag>();
-    trap.assignStar(label);
+  llvm::StringRef trapParent = llvm::sys::path::parent_path(tempTrapPath);
+  if (std::error_code ec = llvm::sys::fs::create_directories(trapParent)) {
+    std::cerr << "Cannot create trap directory '" << trapParent.str() << "': " << ec.message()
+              << "\n";
+    return;
+  }
+
+  std::ofstream trapStream(tempTrapPath.str().str());
+  if (!trapStream) {
+    std::error_code ec;
+    ec.assign(errno, std::generic_category());
+    std::cerr << "Cannot create temp trap file '" << tempTrapPath.str().str()
+              << "': " << ec.message() << "\n";
+    return;
+  }
+  trapStream << "// extractor-args: ";
+  for (auto opt : config.frontendOptions) {
+    trapStream << std::quoted(opt) << " ";
+  }
+  trapStream << "\n\n";
+
+  TrapOutput trap{trapStream};
+  TrapArena arena{};
+  auto label = arena.allocateLabel<File::Tag>();
+  trap.assignStar(label);
+  File f{};
+  f.id = label;
+  f.name = srcFilePath.str().str();
+  trap.emit(f);
+
+  // TODO: Pick a better name to avoid collisions
+  std::string trapName = file.getFilename().str() + ".trap";
+  llvm::SmallString<PATH_MAX> trapPath(config.trapDir);
+  llvm::sys::path::append(trapPath, trapName);
 
-    File f{};
-    f.id = label;
-    f.name = srcFilePath.str().str();
-    trap.emit(f);
+  // TODO: The last process wins. Should we do better than that?
+  if (std::error_code ec = llvm::sys::fs::rename(tempTrapPath, trapPath)) {
+    std::cerr << "Cannot rename temp trap file '" << tempTrapPath.str().str() << "' -> '"
+              << trapPath.str().str() << "': " << ec.message() << "\n";
   }
 }
 

swift/extractor/trap/TrapOutput.h

@@ -1,83 +1,19 @@
 #pragma once
 
 #include <memory>
-#include <llvm/ADT/SmallString.h>
-#include <llvm/Support/FileSystem.h>
-#include <llvm/Support/Path.h>
-
-#include "swift/extractor/SwiftExtractorConfiguration.h"
 #include "swift/extractor/trap/TrapLabel.h"
 
 namespace codeql {
 
 // Sink for trap emissions and label assignments. This abstracts away `ofstream` operations
 // like `ofstream`, an explicit bool operator is provided, that return false if something
 // went wrong
+// TODO better error handling
 class TrapOutput {
-  std::ofstream out_{};
-  std::string target_;
+  std::ostream& out_;
 
  public:
-  // open a output stream. Internally a temporary file is created which is then moved into place
-  // on `close()` or destruction
-  TrapOutput(const SwiftExtractorConfiguration& config, const std::string& target) {
-    // TODO: Pick a better name to avoid collisions
-    llvm::SmallString<PATH_MAX> trapPath(config.trapDir);
-    llvm::sys::path::append(trapPath, target);
-    target_ = trapPath.str().str();
-
-    auto trapDir = llvm::sys::path::parent_path(trapPath);
-    if (std::error_code ec = llvm::sys::fs::create_directories(trapDir)) {
-      std::cerr << "Cannot create trap directory '" << trapDir.str() << "': " << ec.message()
-                << "\n";
-      return;
-    }
-
-    auto tempFile = getTempFile();
-    out_ = std::ofstream{tempFile};
-    if (!out_) {
-      std::error_code ec;
-      ec.assign(errno, std::generic_category());
-      std::cerr << "Cannot create temp trap file '" << tempFile << "': " << ec.message() << "\n";
-      return;
-    }
-    out_ << "// extractor-args: ";
-    for (auto opt : config.frontendOptions) {
-      out_ << std::quoted(opt) << " ";
-    }
-    out_ << "\n\n";
-  }
-
-  ~TrapOutput() { close(); }
-
-  TrapOutput(const TrapOutput& other) = delete;
-  TrapOutput& operator=(const TrapOutput& other) = delete;
-
-  TrapOutput(TrapOutput&& other) : out_{std::move(other.out_)}, target_{std::move(other.target_)} {
-    assert(!other.out_.is_open());
-  }
-
-  TrapOutput& operator=(TrapOutput&& other) {
-    close();
-    out_ = std::move(other.out_);
-    target_ = std::move(other.target_);
-    return *this;
-  }
-
-  explicit operator bool() const { return bool{out_}; }
-
-  void close() {
-    // TODO: The last process wins. Should we do better than that?
-    if (out_.is_open()) {
-      out_.close();
-      auto tempFile = getTempFile();
-      auto targetFile = getTargetFile();
-      if (std::error_code ec = llvm::sys::fs::rename(tempFile, targetFile)) {
-        std::cerr << "Cannot rename temp trap file '" << tempFile << "' -> '" << targetFile
-                  << "': " << ec.message() << "\n";
-      }
-    }
-  }
+  explicit TrapOutput(std::ostream& out) : out_{out} {}
 
   template <typename Tag>
   void assignStar(TrapLabel<Tag> label) {
@@ -104,12 +40,6 @@ class TrapOutput {
   }
 
  private:
-  std::string getTargetFile() { return target_ + ".trap"; }
-  std::string getTempFile() {
-    // TODO: find a more robust approach to avoid collisions?
-    return target_ + '.' + std::to_string(getpid()) + ".trap";
-  }
-
   template <typename... Args>
   void print(const Args&... args) {
     (out_ << ... << args) << '\n';