[Webkit-unassigned] [Bug 63858] DFG JIT does not implement op_call

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=63858


Gavin Barraclough <barraclough at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #99561|review?                     |review-
               Flag|                            |




--- Comment #15 from Gavin Barraclough <barraclough at apple.com>  2011-07-03 18:36:59 PST ---
(From update of attachment 99561)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.