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

[Rafael Espíndola via llvm-commits]

I do like using C if better than #if, but we should change at least
all of lld on one go if we decide to do it.

Cheers,
Rafael


On 17 November 2016 at 16:40, Rui Ueyama <ruiu at google.com> wrote:
> 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(), '\\', '/');
> ----------------
> 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?
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D26734
>
>
>