Skip to content

ReferenceConstructor should accept a single type argument #284

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jasssonpet merged 1 commit into from
Sep 15, 2015
Merged

ReferenceConstructor should accept a single type argument #284

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/NativeScript/Marshalling/Record/RecordInstance.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ class RecordInstance : public JSC::JSDestructibleObject {
return this->_size;
}

PointerInstance* pointer() const {
return this->_pointer.get();
}

private:
RecordInstance(JSC::VM& vm, JSC::Structure* structure)
: Base(vm, structure) {
Expand Down
54 changes: 35 additions & 19 deletions src/NativeScript/Marshalling/Reference/ReferenceConstructor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
#include "ReferenceConstructor.h"
#include "ReferencePrototype.h"
#include "ReferenceInstance.h"
#include "ReferenceTypeInstance.h"
#include "RecordInstance.h"
#include "Interop.h"

namespace NativeScript {
Expand All @@ -27,32 +29,46 @@ static EncodedJSValue JSC_HOST_CALL constructReference(ExecState* execState) {
GlobalObject* globalObject = jsCast<GlobalObject*>(execState->lexicalGlobalObject());
ReferenceInstance* result;

if (execState->argumentCount() == 2) {
JSValue type = execState->uncheckedArgument(0);
JSValue value = execState->uncheckedArgument(1);
JSValue maybeType = execState->argument(0);
const FFITypeMethodTable* ffiTypeMethodTable;
if (tryGetFFITypeMethodTable(maybeType, &ffiTypeMethodTable)) {
void* handle = nullptr;
bool adopted = true;

const FFITypeMethodTable* ffiTypeMethodTable;
if (!tryGetFFITypeMethodTable(type, &ffiTypeMethodTable)) {
return throwVMError(execState, createError(execState, WTF::ASCIILiteral("Not a valid type object is passed as parameter.")));
}
if (execState->argumentCount() == 2) {
JSValue value = execState->uncheckedArgument(1);
if (PointerInstance* pointer = jsDynamicCast<PointerInstance*>(value)) {
Copy link
Copy Markdown
Contributor

@jasssonpet jasssonpet Aug 28, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would happen if the value has a handle, but is not a Pointer. Do you think that this check could be made to tryHandleofValue?

Copy link
Copy Markdown
Contributor Author

@fealebenpae fealebenpae Aug 28, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, ObjCWrapperObject and RecordInstance both have handles. We wouldn't want to dereference the handle of the former, though, so I didn't want to go with tryHandleofValue, although it makes sense to add a case where the value is a RecordInstance so that we can reuse its inner buffer. Save for records, can you think of any other cases where we'd want to reuse buffers?

Copy link
Copy Markdown
Contributor

@jasssonpet jasssonpet Aug 28, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, that the problem with copying occurs with Reference also:

var point = new CGPoint();
var reference1 = new interop.Reference(CGPoint, point);
var reference2 = new interop.Reference(new interop.types.ReferenceType(CGPoint), reference1);

point.x = 2;

console.log(JSON.stringify(point)); // x: 2, y: 0
console.log(JSON.stringify(reference1[0])); // x: 0, y: 0
console.log(JSON.stringify(reference2[0][0])); // x: 0, y: 0

Copy link
Copy Markdown
Contributor Author

@fealebenpae fealebenpae Aug 28, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, RecordInstance and ReferenceInstance can be special-handled like PointerInstance is now?

Copy link
Copy Markdown
Contributor

@jasssonpet jasssonpet Aug 28, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the problem using handleOf? We have a similar snippet in our docs:

var object = new UIView();

var ref = new interop.Reference(interop.types.id, object);
console.log(ref[0]); // isa: UIView
console.log(ref[1]); // _layer: CALayer
console.log(ref[2]); // _gestureInfo: null

Copy link
Copy Markdown
Contributor Author

@fealebenpae fealebenpae Aug 28, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It dereferences the value - the snippet you linked to behaves like a pointer to the NSObject structure and would likely crash if the ivars weren't all ids already. I think the intention of new interop.Reference(interop.types.id, object) is such that ref.value === object ought to be true. What I'm trying to achieve is NSObject **, whereas the current effect is *(NSObject *).

Copy link
Copy Markdown
Contributor

@jasssonpet jasssonpet Aug 28, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I can't think of other types.

handle = pointer->data();
adopted = pointer->isAdopted();
} else if (RecordInstance* record = jsDynamicCast<RecordInstance*>(value)) {
handle = record->pointer()->data();
adopted = record->pointer()->isAdopted();
} else if (ReferenceInstance* reference = jsDynamicCast<ReferenceInstance*>(value)) {
if (maybeType.inherits(ReferenceTypeInstance::info())) {
// do nothing, this is a reference to reference
} else if (PointerInstance* pointer = reference->pointer()) {
handle = pointer->data();
adopted = pointer->isAdopted();
} else {
value = reference->get(execState, execState->propertyNames().value);
}
}

bool hasHandle;
void* handle = tryHandleofValue(value, &hasHandle);
PointerInstance* pointer;
if (hasHandle) {
pointer = jsCast<PointerInstance*>(globalObject->interop()->pointerInstanceForPointer(execState->vm(), handle));
if (!handle) {
handle = calloc(ffiTypeMethodTable->ffiType->size, 1);
ffiTypeMethodTable->write(execState, value, handle, maybeType.asCell());
}
} else {
handle = calloc(ffiTypeMethodTable->ffiType->size, 1);
pointer = jsCast<PointerInstance*>(globalObject->interop()->pointerInstanceForPointer(execState->vm(), handle));
pointer->setAdopted(true);

ffiTypeMethodTable->write(execState, value, handle, type.asCell());
}

result = ReferenceInstance::create(execState->vm(), globalObject, globalObject->interop()->referenceInstanceStructure(), type.asCell(), pointer);
PointerInstance* pointer = jsCast<PointerInstance*>(globalObject->interop()->pointerInstanceForPointer(execState->vm(), handle));
pointer->setAdopted(adopted);
result = ReferenceInstance::create(execState->vm(), globalObject, globalObject->interop()->referenceInstanceStructure(), maybeType.asCell(), pointer);
} else if (execState->argumentCount() == 2) {
return throwVMError(execState, createError(execState, WTF::ASCIILiteral("Not a valid type object is passed as parameter.")));
} else {
const JSValue value = execState->argument(0);
result = ReferenceInstance::create(execState->vm(), globalObject->interop()->referenceInstanceStructure(), value);
result = ReferenceInstance::create(execState->vm(), globalObject->interop()->referenceInstanceStructure(), maybeType);
}

return JSValue::encode(result);
Expand Down
4 changes: 4 additions & 0 deletions src/NativeScript/Marshalling/Reference/ReferenceInstance.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ class ReferenceInstance : public JSC::JSDestructibleObject {
return this->_pointer ? this->_pointer->data() : nullptr;
}

PointerInstance* pointer() const {
return this->_pointer.get();
}

JSC::JSCell* innerType() const {
return this->_innerTypeCell.get();
}
Expand Down
74 changes: 74 additions & 0 deletions tests/TestRunner/app/Marshalling/ReferenceTests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,4 +229,78 @@ describe(module.id, function () {

expect(array.firstObject).toBe(object);
});

describe("ReferenceConstructor", function () {
it("should accept empty arguments", function () {
var reference = new interop.Reference();

expect(reference).toEqual(jasmine.any(interop.Reference));
});

it("should accept a single value argument", function () {
var value = "value";

var reference = new interop.Reference(value);

expect(reference).toEqual(jasmine.any(interop.Reference));
expect(reference.value).toBe(value);
});

it("should accept a single type argument", function () {
var reference = new interop.Reference(interop.types.bool);

expect(reference).toEqual(jasmine.any(interop.Reference));
expect(interop.handleof(reference)).toEqual(jasmine.any(interop.Pointer));
});

it("should accept type and value arguments", function () {
var value = NSObject.alloc().init();
var reference = new interop.Reference(NSObject, value);
var buffer = interop.handleof(reference);

expect(reference).toEqual(jasmine.any(interop.Reference));
expect(reference.value).toBe(value);
expect(buffer).toEqual(jasmine.any(interop.Pointer));
});

it("should accept type and pointer arguments", function () {
var pointer = interop.alloc(1);
var reference = new interop.Reference(interop.types.bool, pointer);

expect(reference).toEqual(jasmine.any(interop.Reference));
expect(interop.handleof(reference)).toBe(pointer);
});

it("should accept type and record arguments", function () {
var record = new CGPoint();
var reference = new interop.Reference(CGPoint, record);

expect(reference).toEqual(jasmine.any(interop.Reference));
expect(interop.handleof(reference)).toBe(interop.handleof(record));
});

it("should accept type and pointer-backed reference arguments", function () {
var ref = new interop.Reference(interop.types.bool);
var reference = new interop.Reference(interop.types.bool, ref);

expect(reference).toEqual(jasmine.any(interop.Reference));
expect(interop.handleof(reference)).toBe(interop.handleof(ref));
});

it("should accept type and uninitialized reference arguments", function () {
var ref = new interop.Reference(123);
var reference = new interop.Reference(interop.types.int8, ref);

expect(reference).toEqual(jasmine.any(interop.Reference));
expect(ref.value).toEqual(reference.value);
});

it("should accept reference type and reference arguments", function () {
var ref = new interop.Reference(interop.types.bool);
var reference = new interop.Reference(new interop.types.ReferenceType(interop.types.bool), ref);

expect(reference).toEqual(jasmine.any(interop.Reference));
expect(interop.handleof(reference.value)).toBe(interop.handleof(ref));
});
});
});