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.
