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

[Filipe Cabecinhas via llvm-commits]

filcab added inline comments.


================
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(), '\\', '/');
----------------
ruiu wrote:
> davide wrote:
> > davide wrote:
> > > ruiu wrote:
> > > > filcab wrote:
> > > > > Nit: How about `if (LLVM_ON_WIN32)`?
> > > > > It would force you to always be able to compile the code :-)
> > > > Is that a common practice? I believe for #define'd macros, we usually use #ifdef.
> > > Probably, but we should change also in the Archiver at that point.
> > Just checked, and noticed we're inconsistent here.
> > 
> > ```
> > [davide at cupiditate lib]$ grep -R 'if LLVM_ON_WIN32' * | wc -l
> > 8
> > [davide at cupiditate lib]$ grep -R 'ifdef LLVM_ON_WIN32' * | wc -l
> > 29
> > ```
> > So, Filipe, you prefer the other spelling? If so, I can change, but I'd rather change it everywhere.
> I was thinking that Filipe suggested use C's `if` instead of C preprocessor's #ifdef (which I don't agree).
I'd prefer the `if (LLVM_ON_WIN32)` variant, as it doesn't allow for code that doesn't compile. But if the regular contributors have objections, I don't mind it too much.


================
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(), '\\', '/');
----------------
filcab wrote:
> ruiu wrote:
> > davide wrote:
> > > davide wrote:
> > > > ruiu wrote:
> > > > > filcab wrote:
> > > > > > Nit: How about `if (LLVM_ON_WIN32)`?
> > > > > > It would force you to always be able to compile the code :-)
> > > > > Is that a common practice? I believe for #define'd macros, we usually use #ifdef.
> > > > Probably, but we should change also in the Archiver at that point.
> > > Just checked, and noticed we're inconsistent here.
> > > 
> > > ```
> > > [davide at cupiditate lib]$ grep -R 'if LLVM_ON_WIN32' * | wc -l
> > > 8
> > > [davide at cupiditate lib]$ grep -R 'ifdef LLVM_ON_WIN32' * | wc -l
> > > 29
> > > ```
> > > So, Filipe, you prefer the other spelling? If so, I can change, but I'd rather change it everywhere.
> > I was thinking that Filipe suggested use C's `if` instead of C preprocessor's #ifdef (which I don't agree).
> I'd prefer the `if (LLVM_ON_WIN32)` variant, as it doesn't allow for code that doesn't compile. But if the regular contributors have objections, I don't mind it too much.
ruiu: It's very common in the sanitizers, and it ends up being cleaner and safer.


================
Comment at: lld/trunk/test/ELF/reproduce-backslash.s:1
+# REQUIRES: x86, cpio, shell
+
----------------
ruiu wrote:
> davide wrote:
> > filcab wrote:
> > > Can `shell` be replaced by `UNSUPPORTED: system-windows`, or are you relying on having a "proper shell" and I'm missing something?
> > I've seen this used in general to constrain tests to be ran only on Windows (at least in lld).
> `shell` is used to imply "not on Windows", though it is indeed confusing. Windows has shells (command prompt and explorer), of course, so we probably should define "unix", "posix" or something like that to identify Unix.
Indeed: `shell` is not used very well on llvm. I think it would be nice to get this test right (especially since it might be run under Windows with a "shell"), but I don't think we should refactor all tests right now.  (And if this test ends up using `shell` to mean "Windows", it's not the worst thing.)

Refactoring the features should probably be discussed in llvm-dev first, since we'll need to nail down the semantics correctly for the several cases we need (and make it consistent, which is the biggest problem).


Repository:
  rL LLVM

https://reviews.llvm.org/D26734