[PATCH] D19737: Make --reproduce produce more reproducible logs.

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/DriverUtils.cpp:122
@@ +121,3 @@
+// okay because we don't want to have a complicated logic
+// for a debugging feature.
+static std::string quote(StringRef S) {
----------------
silvas wrote:
> Clang generates response files for the linker and has been in production for some time. Can we share that code? I think the code we want to share is part of Command::writeResponseFile.
I'll try.

================
Comment at: ELF/DriverUtils.cpp:144
@@ +143,3 @@
+  SmallString<128> Path;
+  path::append(Path, Config->Reproduce, "invocation.txt");
+  std::error_code EC;
----------------
silvas wrote:
> You may want to call this `invocation.rsp` since it is a response file (it obviously doesn't matter, but since we are changing the meaning from the previous `invocation.txt` it might help to avoid confusion).
How about response.txt? I think on Windows "rsp" extension is not recognized so that it is not easy to look inside.

================
Comment at: ELF/DriverUtils.cpp:152
@@ +151,3 @@
+  for (auto *Arg : Args) {
+    switch (Arg->getOption().getID()) {
+    case OPT_reproduce:
----------------
silvas wrote:
> Nice reusing libOption for this! This is really nice!
:)

================
Comment at: test/ELF/reproduce.s:20
@@ -16,2 +19,3 @@
 # RUN: ld.lld ./../../../foo.o -o bar -shared --as-needed --reproduce repro
-# RUN: diff %t.dir/build2/foo.o repro/foo.o
+# RUN: diff %t.dir/build2/foo.o repro/%t.dir/build2/foo.o
+
----------------
silvas wrote:
> When I tried the first version of your patch this was failing on windows for the same reason as the original reproduce patch did (lit expands a path `foo/C:/...`). Can we avoid the problem?
Reid suggested defining a new variable "%:t" to get a path without a drive letter. But I'm not sure if it's worth to do. I'm inclined to simply add "REQUIRES: shell" to this file.


http://reviews.llvm.org/D19737