[webkit-reviews] review denied: [Bug 63858] DFG JIT does not implement op_call : [Attachment 99561] the path (address Mark's comment)

[bugzilla-daemon at webkit.org]

Gavin Barraclough <barraclough at apple.com> has denied Filip Pizlo
<fpizlo at apple.com>'s request for review:
Bug 63858: DFG JIT does not implement op_call
https://bugs.webkit.org/show_bug.cgi?id=63858

Attachment 99561: the path (address Mark's comment)
https://bugs.webkit.org/attachment.cgi?id=99561&action=review

------- Additional Comments from Gavin Barraclough <barraclough at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=99561&action=review

r- for a couple of minor nits, but on the whole looking really great.

> Source/JavaScriptCore/assembler/CodeLocation.h:122
> +class CodeLocationPossiblyNearCall : public CodeLocationCommon {

I'm not sure this extra type really adds any value.  If you want to change the
type of the location held in the CodeBlock, you could just make it a
CodeLocationLabel (after all, this is just a pointer into the instruction
stream).  It seems highly unlikely this class will provide much value going
forwards, since (1) we're likely to deprecate the old JIT, and (2) the new JIT
is likely want to make near calls for JIT->JIT calls.  Still, I don't feel
strongly about this - if it becomes redundant, it can be removed then.	So I'd
recommend removing, but feel free to leave as is if you prefer.

> Source/JavaScriptCore/dfg/DFGOperations.cpp:391
> +EncodedJSValue returnValueHack();

We don't like names like like with 'hack' in them - it doesn't convey any
helpful information.  I'd suggest 'getHostCallReturnValue' would be a more
descriptive function name here.

> Source/JavaScriptCore/interpreter/CallFrame.h:41
> +	   JSValue calleeAsValue() const { return
this[RegisterFile::Callee].jsValue(); }

I don't think this method should be needed; you should just be able to call
'callee()', and assign the result to a JSValue.  Furthermore, this may be
incorrect - I'm not sure that we write the tag value for the callee on
JVALUE32_64, we may just write the JSCell*, so reading the full 64-bit entry
may result in an incorrect tag being read.  Did using 'callee()' cause any
problems?

> Source/JavaScriptCore/runtime/JSFunction.h:40
> +    }

I don't think we normally doubly indent nested namespace declarations, this
should probably all lose an indent level.