Bug 95418 – Build warning : -Wsign-compare on DFGByteCodeParser.cpp.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Byungwoo Lee]

There is a build warning '-Wsign-compare' on findArgumentPositionForLocal() function in DFGByteCodeParser.cpp

[Byungwoo Lee]

Created attachment 161393 [details]
Patch

[Kentaro Hara]

Comment on attachment 161393 [details]
Patch

OK

[Build Bot]

Comment on attachment 161393 [details]
Patch

Attachment 161393 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13681523

[Filip Pizlo]

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.

[Byungwoo Lee]

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 :)

[Filip Pizlo]

(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.

[Byungwoo Lee]

Created attachment 161408 [details]
Patch

[Filip Pizlo]

Comment on attachment 161408 [details]
Patch

This looks good.  Hopefully it will pass all the bots!

[Byungwoo Lee]

All the bots were passed. Thanks for your reviews. :)

[WebKit Review Bot]

Comment on attachment 161408 [details]
Patch

Clearing flags on attachment: 161408

Committed r127167: <http://trac.webkit.org/changeset/127167>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.