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

[Filipe Cabecinhas via llvm-commits]

Indeed, consistency is important. Anyway, I've already withdrew my
comment (especially since I'm not an active contributor to lld, so no
sense imposing my stylistic choices without much discussion). No need
to keep derailing this thread. We can discuss changing it throughout
lld (if it's desired) in its own thread.

Thank you,
 Filipe

On Thu, Nov 17, 2016 at 9:47 PM, Rafael Espíndola
<rafael.espindola at gmail.com> wrote:
> 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
>>
>>
>>