[PATCH] D19494: [ELF] Introduce --reproduce flag

[Rui Ueyama via llvm-commits]

ruiu added a subscriber: ruiu.

================
Comment at: ELF/Config.h:51
@@ -50,2 +50,3 @@
   std::string RPath;
+  std::string ReproduceDir = "";
   std::vector<llvm::StringRef> DynamicList;
----------------
Remove `= ""`. Strings are initialized to empty string by default. Also please rename `Reproduce` as the members of Config have the same name as command line options.

================
Comment at: ELF/Driver.cpp:101-107
@@ -99,1 +100,9 @@
 
+// Should this be moved to LLVM/Support ?
+static std::string getFullPath(StringRef FileName) {
+  SmallString<128> CurPath;
+  sys::fs::current_path(CurPath);
+  sys::path::append(CurPath, FileName);
+  return CurPath.str();
+}
+
----------------
Why do you need to make a path full path?

================
Comment at: ELF/Driver.cpp:115
@@ +114,3 @@
+  EC = llvm::sys::fs::create_directories(
+    Config->ReproduceDir + "/" + llvm::sys::path::parent_path(SrcPath));
+  if (EC) {
----------------
I think you want to use path::append instead of appending "/".

================
Comment at: ELF/Driver.cpp:118
@@ +117,3 @@
+    error("--reproduce: can't create directory " +
+        Config->ReproduceDir + "/" + SrcPath
+        + ": " + EC.message());
----------------
Let's create a std::string for the directory name so that we don't need to repeat this string concatenation twice.

================
Comment at: ELF/Driver.cpp:135
@@ -105,1 +134,3 @@
     llvm::outs() << Path << "\n";
+  dumpFile(Path);
+
----------------
Let's make it explicit that the feature is optional.

  if (Config->Reproduce)
    dumpFile(Path);

================
Comment at: ELF/Driver.cpp:263
@@ +262,3 @@
+  if (EC) {
+    error("--reproduce: can't create directory: " + EC.message());
+    return;
----------------
  error(EC, "--reproduce: can't create directory");

================
Comment at: ELF/Driver.cpp:270
@@ +269,3 @@
+  check(EC);
+  for (auto Arg: Args)
+    OS << Arg << " ";
----------------
auto -> const char *

================
Comment at: ELF/Driver.cpp:271
@@ +270,3 @@
+  for (auto Arg: Args)
+    OS << Arg << " ";
+}
----------------
This would leave a space character at end of the line and does not terminate the line with '\n'. As this is not a speed-critical path, I'd do

  OS << llvm::join(Args.begin(), Args.end(), " ") << "\n";

================
Comment at: ELF/Driver.cpp:288
@@ -239,2 +287,3 @@
   readConfigs(Args);
+  dumpLinkerInvocation(ArgsArr);
   createFiles(Args);
----------------
Guard with `if (Config->Reproduce)` as well.

================
Comment at: ELF/Driver.cpp:436-437
@@ +435,4 @@
+      Config->ReproduceDir = Arg->getValue();
+    if (llvm::sys::fs::exists(Config->ReproduceDir))
+      error("--reproduce: directory " + Config->ReproduceDir + " already exists.");
+  }
----------------
This error would be caught in dumpLinkerInvocation, no?


http://reviews.llvm.org/D19494