Re: [PATCH] canonicalize: add support for not resolving symlinks

[Jim Meyering]

Pádraig Brady wrote:
> On 12/30/2011 02:43 PM, Pádraig Brady wrote:
>> On 12/30/2011 01:49 PM, Eric Blake wrote:
>>> On 12/30/2011 04:04 AM, Jim Meyering wrote:
>>>>> -          if (lstat (rname, &st) != 0)
>>>>> +          if ((logical?stat:lstat) (rname, &st) != 0)
>>>>
>>>> Please add spaces around operators:
>>>>
>>>>              if ((logical ? stat : lstat) (rname, &st) != 0)
>>>
>>> Better yet, don't write this as a function pointer conditional, as the
>>> gnulib replacement for stat on some platforms has to be a function-like
>>> macro (no thanks to 'struct stat').  Using a function pointer
>>> conditional risks missing the gnulib macro for stat, and calling the
>>> underlying system stat() instead of the intended rpl_stat(), for broken
>>> behavior.  You have to use:
>>>
>>> if (logical ? stat (rname, &st) : lstat (rname, &st)) != 0)
>>>
>>
>> Ouch. Good catch.
>> I'll need to fix in a follow up patch.
>> I'll need to adjust such uses in coreutils too.
>
> Actually there is a syntax-check for this in coreutils.
> Hence there are no bad uses there.
>
> The snippet I used above was cut and pasted from a comment,
> which wasn't apparent as I used `git grep` to search for the
> preferred form of the idiom. I thought it a bit strange
> that there were no spaces around the operators but didn't
> investigate further :)

This is a good argument for not leaving bad code in comments.
At least not bad code that is alone on a line.
It's easy to miss the lack of a trailing semicolon.