Skip to content
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.

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

Support setting list items #46

Merged
merged 7 commits into from
Oct 13, 2015
Merged

Support setting list items #46

merged 7 commits into from
Oct 13, 2015

Conversation

appden
Copy link
Contributor

@appden appden commented Oct 12, 2015

Fixes #45

}

size_t index = std::stol(indexStr);
if (index >= size) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we handle negative indexes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but actually does the C++ check for that like it does in the case of the getters? If so, then I could just remove this check altogether.

Copy link
Contributor

Choose a reason for hiding this comment

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

C++ does not handle negative indexes, nor does it do any error checking at the core level. The object store/shared_realm code should check all arguments but we are interacting with the link_view directly here which is at the core level. We could/should put this check in a method on the array class (but can do this later), but support for negative indexes should be done in our binding code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on our slack discussion, if we remove this check then ObjectArray should handle this for us?

@alazier
Copy link
Contributor

alazier commented Oct 12, 2015

Please rebase/merge master if you haven't already to see if my earlier change fixes the CI failures.

This must have been accidentally copy-pasted from ObjectTests.js
Added a test and made other array tests syntactically consistent. Resolves #45
@@ -261,7 +261,7 @@ JSValueRef RealmCreateObject(JSContextRef ctx, JSObjectRef function, JSObjectRef

bool update = false;
if (argumentCount == 3) {
update = RJSValidatedValueToBool(ctx, arguments[2]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did we change this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the same idea. If something expects a Boolean in JS, it never should throw just because you passed null or 0 or whatever. Minifiers often take advantage of that assumption to convert all false to 0, etc.

Turns out this API can return NaN without an exception. Also added tests to make sure these conversions either work or throw exceptions in the appropriate places.
@alazier
Copy link
Contributor

alazier commented Oct 13, 2015

👍

appden added a commit that referenced this pull request Oct 13, 2015
Fixes many list-related issues.
@appden appden merged commit aa3e40f into master Oct 13, 2015
@appden appden deleted the sk-array-setter branch October 13, 2015 00:18
alazier pushed a commit that referenced this pull request Feb 23, 2016
alazier pushed a commit that referenced this pull request Sep 14, 2016
* Update sync dependency

* Bump version to 0.14.0-1

* Bump sync version for testing

* Bump sync version
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants