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

[Rui Ueyama via llvm-commits]

ruiu 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:
> filcab wrote:
> > filcab wrote:
> > > 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.
> > Since Rui doesn't seem to agree, I'll withdraw this comment.
> At LLVM top directory:
> 
> $ git grep '#if.*LLVM_ON_WIN32'|wc -l
> 67
> 
> $ git grep 'if (LLVM_ON_WIN32)' | wc -l
> 0
> 
> Is this common?
Are you suggesting

  #if LLVM_ON_WIN32

instead of

  #ifdef LLVM_ON_WIN32

?

Or,

  static void convertToUnixPathSeparator(SmallString<128> &Path) {
    if (LLVM_ON_WIN32)
      std::replace(Path.begin(), Path.end(), '\\', '/');
  }

?


Repository:
  rL LLVM

https://reviews.llvm.org/D26734