There is a build warning '-Wsign-compare' on findArgumentPositionForLocal() function in DFGByteCodeParser.cpp
Created attachment 161393 [details] Patch
Comment on attachment 161393 [details] Patch OK
Comment on attachment 161393 [details] Patch Attachment 161393 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13681523
Comment on attachment 161393 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161393&action=review I think what you're hitting here is that different compilers have different opinions about what the sign of arithmetic on a part signed, part unsigned expression is. On our build system, the current setup works - these comparisons are all signed versus signed. If your build system disagrees (which it apparently is doing), you should insert casts into the arithmetic (inlineCallFrame->stackOffset + CallFrame::thisArgumentOffset(), and inlineCallFrame->stackOffset - RegisterFile::CallFrameHeaderSize - inlineCallFrame->arguments.size()) to ensure that it stays signed all the way through. And please don't change operand variables to unsigned. Operands are always signed, since negative operand values carry special meaning. > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:362 > - ArgumentPosition* findArgumentPositionForLocal(int operand) > + ArgumentPosition* findArgumentPositionForLocal(unsigned operand) This is wrong. Operands are always signed integers. > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:370 > - if (operand == inlineCallFrame->stackOffset + CallFrame::thisArgumentOffset()) > + if (static_cast<int>(operand) == inlineCallFrame->stackOffset + CallFrame::thisArgumentOffset()) Change the right hand side of this to static_cast<int>(inlineCallFrame->...) > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:372 > - if (static_cast<unsigned>(operand) < inlineCallFrame->stackOffset - RegisterFile::CallFrameHeaderSize - inlineCallFrame->arguments.size()) > + if (operand < inlineCallFrame->stackOffset - RegisterFile::CallFrameHeaderSize - inlineCallFrame->arguments.size()) Same as above. Change the right hand side to a cast to int. > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:374 > - int argument = operandToArgument(operand - inlineCallFrame->stackOffset); > + int argument = operandToArgument(static_cast<int>(operand) - inlineCallFrame->stackOffset); Remove this.
Comment on attachment 161393 [details] Patch Thanks for your comments to arrange this. I just changed the type of parameter because that, current implementation looks not having the case that a negative value is passed through that parameter. But now I understand your point about the special meaning of the type of the 'operand'. I'll remain the parameter type and make new patch according to your guide. Thanks :)
(In reply to comment #5) > (From update of attachment 161393 [details]) > Thanks for your comments to arrange this. > I just changed the type of parameter because that, current implementation looks not having the case that a negative value is passed through that parameter. You are absolutely correct. I just think it's better to maintain consistency here. > But now I understand your point about the special meaning of the type of the 'operand'. > I'll remain the parameter type and make new patch according to your guide. > Thanks :) Cool! Thanks for fixing these issues.
Created attachment 161408 [details] Patch
Comment on attachment 161408 [details] Patch This looks good. Hopefully it will pass all the bots!
All the bots were passed. Thanks for your reviews. :)
Comment on attachment 161408 [details] Patch Clearing flags on attachment: 161408 Committed r127167: <http://trac.webkit.org/changeset/127167>
All reviewed patches have been landed. Closing bug.