[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:
> 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(), '\\', '/');
>   }
> 
> ?
It's probably a sanitizer thing, then.
More common in the compiler-rt repo (but we still have a ton of ifdefs for cases when one of the parts won't compile on "the other" system, for example).


Repository:
  rL LLVM

https://reviews.llvm.org/D26734