[webkit-reviews] review denied: [Bug 115022] [SH4] Misc bugfix and cleaning in sh4 base JIT : [Attachment 199219] Misc bugfix and cleaning in sh4 base JIT (3)

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has denied Julien Brianceau
<jbrianceau at nds.com>'s request for review:
Bug 115022: [SH4] Misc bugfix and cleaning in sh4 base JIT
https://bugs.webkit.org/show_bug.cgi?id=115022

Attachment 199219: Misc bugfix and cleaning in sh4 base JIT (3)
https://bugs.webkit.org/attachment.cgi?id=199219&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=199219&action=review


>>> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:1920
>>> +		     result = (cond == NonZero) ? false : true;
>> 
>> I’d suggest simplifying this to "result = (cond != NonZero);”.
> 
> I thought of that, but I think it's easier to read and understand with the
current version: if (cond == NonZero) we get a branchFalse, and a branchTrue
otherwise. Of course I'm not opposed to change that if you think this is
better.

I disagree, result = cond != NonZero; is much clearer (there aren't other
places where we use ?: on a boolean operation to distinguish true from false

An alternative would be
enum { BranchTrue, BranchFalse } result;
...
return result == BranchTrue ? branchTrue() : branchFalse();

That might be the cleanest and clearest