On Mon, Jan 11, 2021 at 12:15:58PM -0500, Derrick Stolee wrote: > On 1/11/2021 11:30 AM, Taylor Blau wrote: > > On Mon, Jan 11, 2021 at 07:07:17AM -0500, Derrick Stolee wrote: > >> My comments on this series are very minor. > >> > >> I made only one comment about "if (method() < 0)" versus > >> "if (method())" but that pattern appears in multiple patches. > >> _If_ you decide to change that pattern, then I'm sure you can > >> find all uses. > > > > I have no strong opinion here, so I'm happy to defer to your or others' > > judgement. My very weak opinion is that I'd just as soon leave it as-is, > > but that if I'm rerolling and others would like to see it changed, then > > I'm happy to do it. > > Well, I found 782 instances of ") < 0)" in the codebase, and my initial > scan of these shows they are doing exactly what you are asking. So as > far as code style goes, there is plenty of precedent. Thanks for looking, I was curious about that myself after our thread, but I hadn't yet bothered to look. > The thing that makes me react to this is that it _looks_ like an extra > comparison. However, I'm sure the assembly instructions have the same > performance characteristics between "!= 0" and "< 0". It should make no difference. Both comparisons will do a 'cmp $0 ...' where '...' is probably an indirect into the current frame. The '!= 0' will use je, and the '< 0' comparison will use 'jns'. Both conditional jumps should be implemented by checking a CPU flag only (ZF and SF, respectively). Not that any of this matters, it's just fun to look. > Thanks, > -Stolee > Thanks, Taylor
