Skip to content

ReferenceConstructor should accept a single type argument#284

Merged
jasssonpet merged 1 commit intomasterfrom
fealebenpae/ReferenceConstructor-argument-handling
Sep 15, 2015
Merged

ReferenceConstructor should accept a single type argument#284
jasssonpet merged 1 commit intomasterfrom
fealebenpae/ReferenceConstructor-argument-handling

Conversation

@fealebenpae
Copy link
Copy Markdown
Contributor

and allocate an internal buffer of the correct size.

@ns-bot
Copy link
Copy Markdown

ns-bot commented Aug 28, 2015

Copy link
Copy Markdown
Contributor

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

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

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

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

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

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

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.

@fealebenpae fealebenpae added this to the 1.4.0 (Under review) milestone Sep 14, 2015
@fealebenpae fealebenpae self-assigned this Sep 14, 2015
@fealebenpae
Copy link
Copy Markdown
Contributor Author

run ci

@ns-bot
Copy link
Copy Markdown

ns-bot commented Sep 14, 2015

and allocate an internal buffer of the correct size.
@fealebenpae fealebenpae force-pushed the fealebenpae/ReferenceConstructor-argument-handling branch from 8cc1d31 to 426e4e9 Compare September 14, 2015 16:00
@fealebenpae
Copy link
Copy Markdown
Contributor Author

@jasssonpet: I've updated the patch with handling for the cases we discussed, please take a look.

@ns-bot
Copy link
Copy Markdown

ns-bot commented Sep 14, 2015

@jasssonpet
Copy link
Copy Markdown
Contributor

Looks good to me, 👍

jasssonpet added a commit that referenced this pull request Sep 15, 2015
…uctor-argument-handling

ReferenceConstructor should accept a single type argument
@jasssonpet jasssonpet merged commit 20e3b91 into master Sep 15, 2015
@jasssonpet jasssonpet deleted the fealebenpae/ReferenceConstructor-argument-handling branch September 15, 2015 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants