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

Implement rb_hash_bulk_insert #3715

Merged
merged 1 commit into from
Nov 12, 2024

Conversation

Th3-M4jor
Copy link
Contributor

Closes #3705

In this case I'm not sure if the overhead of repeated calls to []= makes it worth instead adding another method to CExtNodes.java

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Nov 10, 2024
@Th3-M4jor Th3-M4jor force-pushed the implement-rb_hash_bulk_insert branch from c40cf04 to 75f5120 Compare November 10, 2024 23:38
@andrykonchin
Copy link
Member

I believe it's OK to have this overhead.

Copy link
Member

@andrykonchin andrykonchin left a comment

Choose a reason for hiding this comment

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

Good job!

spec/ruby/optional/capi/ext/hash_spec.c Show resolved Hide resolved
hash.should == {a: 1, b: 2}
end
end

Copy link
Member

@andrykonchin andrykonchin Nov 11, 2024

Choose a reason for hiding this comment

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

I would also add the following cases:

  • length isn't even - there should be error raised (I suppose)
  • how duplicated keys (provided in the array) are handled - I suppose they should just override each other
 * What happens for  duplicated keys?  Well it silently discards  older ones to
 * accept the newest (rightmost) one.  This behaviour also mimics repeated call
 * of rb_hash_aset().
  • it's allowed to pass NULL as array if length argument = 0 (as far as it's explicitly stated in a documentation comment
 * @note        `argv` is allowed to be NULL as long as `argc` is zero.

Copy link
Contributor Author

@Th3-M4jor Th3-M4jor Nov 11, 2024

Choose a reason for hiding this comment

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

I'll add tests for the other cases.

As for when the length isn't even, it appears to be that if RUBY_DEBUG is not defined MRI accepts an array with an odd number of elements and will read one past the end. Should we allow the same to maintain compatibility?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, right. Yes, it makes sense to do the same and to allow passing odd length.

Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't add a spec for that odd case, because specs should also pass with RUBY_DEBUG, notably ruby-dev-builder depends on that.
@andrykonchin Could you add a commit removing that spec?

We could file an issue at https://bugs.ruby-lang.org/, I think it would make sense, it doesn't seem OK the C API reads past the length it's given.

@andrykonchin andrykonchin changed the title implement rb_hash_bulk_insert Implement rb_hash_bulk_insert Nov 11, 2024
@Th3-M4jor Th3-M4jor force-pushed the implement-rb_hash_bulk_insert branch from 75f5120 to f39d25d Compare November 11, 2024 23:28
Copy link
Member

@andrykonchin andrykonchin left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you.

@andrykonchin andrykonchin added the in-ci The PR is being tested in CI. Do not push new commits. label Nov 12, 2024
@andrykonchin andrykonchin self-assigned this Nov 12, 2024
@andrykonchin andrykonchin force-pushed the implement-rb_hash_bulk_insert branch from f39d25d to 529bd60 Compare November 12, 2024 11:48
@graalvmbot graalvmbot merged commit 56e0648 into oracle:master Nov 12, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-ci The PR is being tested in CI. Do not push new commits. OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rb_hash_bulk_insert() is not exposed in ruby.h
4 participants