[PATCH] D26734: [ELF] Don't replace path separators on *NIX

[Filipe Cabecinhas via llvm-commits]

filcab added a comment.

I'm a bit late because I forgot to submit the comments.

You're missing a Windows test which shows you're replacing the path components.
Probably with `REQUIRES: system-windows, x86, cpio`

P.S: Sorry for the old, "not visible" comments. I can't get phabricator to delete them.



================
Comment at: lib/Core/Reproduce.cpp:55
 // both unix and windows.
+#ifdef LLVM_ON_WIN32
 static void convertToUnixPathSeparator(SmallString<128> &Path) {
----------------
rafael wrote:
> Move the ifdef inside the function so that we only need one.
You can even do `if (LLVM_ON_WIN32)` and get rid of the `ifdef`s.
Ends up making sure code always compiles :-)


================
Comment at: lld/trunk/lib/Core/Reproduce.cpp:56
 static void convertToUnixPathSeparator(SmallString<128> &Path) {
+#ifdef LLVM_ON_WIN32
   std::replace(Path.begin(), Path.end(), '\\', '/');
----------------
Nit: How about `if (LLVM_ON_WIN32)`?
It would force you to always be able to compile the code :-)


================
Comment at: lld/trunk/test/ELF/reproduce-backslash.s:1
+# REQUIRES: x86, cpio, shell
+
----------------
Can `shell` be replaced by `UNSUPPORTED: system-windows`, or are you relying on having a "proper shell" and I'm missing something?


================
Comment at: test/ELF/reproduce-backslash.s:7
+# RUN: mkdir -p %t.dir/build
+# RUN: llvm-mc %s -o %t.dir/build/foo\\.o -filetype=obj -triple=x86_64-pc-linux
+# RUN: cd %t.dir
----------------
This probably won't work on Windows, though.
`UNSUPPORTED: system-windows`, or something?


Repository:
  rL LLVM

https://reviews.llvm.org/D26734