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

fix shuffle_random #5338

Merged
merged 1 commit into from Oct 11, 2018

Conversation

Projects
None yet
3 participants
@ahorek
Copy link
Contributor

ahorek commented Sep 30, 2018

No description provided.

@kares

This comment has been minimized.

Copy link
Member

kares commented Oct 1, 2018

great, would be nice to get more descriptive from time-to-time :)
... and if you try to align .java style so its more consistent (I also struggle with this on occasions)

try {
while (i > 0) {
int r = (int) RubyRandom.randomLongLimited(context, randgen, i - 1);
if(len != realLength) { // || ptr != RARRAY_CONST_PTR(ary)

This comment has been minimized.

Copy link
@kares

kares Oct 1, 2018

Member

maybe have a ' ' (space) between if and opening (

@@ -714,6 +718,11 @@ public static long randomLongLimited(ThreadContext context, IRubyObject obj, lon
RandomType rnd = tryGetRandomType(context, obj);

if (rnd == null) {
DynamicMethod method = obj.getMetaClass().searchMethod("rand");
if(method.isUndefined() || method.getVisibility() != PUBLIC) {
Helpers.callMethodMissing(context, obj, method.getVisibility(), "rand", CallType.FUNCTIONAL, Block.NULL_BLOCK);

This comment has been minimized.

Copy link
@kares

kares Oct 1, 2018

Member

suddenly indent seems only 2 spaces here, while you (properly) did 4 above

@@ -714,6 +718,11 @@ public static long randomLongLimited(ThreadContext context, IRubyObject obj, lon
RandomType rnd = tryGetRandomType(context, obj);

if (rnd == null) {
DynamicMethod method = obj.getMetaClass().searchMethod("rand");

This comment has been minimized.

Copy link
@ahorek

ahorek Oct 1, 2018

Author Contributor

basically I need a rb_funcallv_public here, but I didn't find a reference for it in jruby, Probably this could be simplified? What do you think @kares ?
ruby/ruby@74ba0cf#diff-a962031992b7b5934db41bf925b3487eL1021

This comment has been minimized.

Copy link
@kares

kares Oct 1, 2018

Member

yeah, MRI isn't modelled 100% ...
think if you do some form of bare "invoke" it should check (public) visibility - seems like that was the point.
maybe try changing Helpers.invoke bellow to a call doing the check: Helpers.invokeFrom ...

@ahorek ahorek force-pushed the ahorek:fix_shuffle branch from b989743 to 4e952bb Oct 1, 2018

@kares kares added this to the JRuby 9.2.1.0 milestone Oct 1, 2018

@kares kares added the core label Oct 1, 2018

@ahorek ahorek force-pushed the ahorek:fix_shuffle branch from 4e952bb to 7791266 Oct 2, 2018

pavel

@ahorek ahorek force-pushed the ahorek:fix_shuffle branch from 7791266 to 389a0ec Oct 2, 2018

@ahorek

This comment has been minimized.

Copy link
Contributor Author

ahorek commented Oct 3, 2018

Ok, I've introduced invokePublic (mri uses it only in a few places)

it's much cleaner now. ping @kares?

@headius headius merged commit b3b85bc into jruby:master Oct 11, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@headius

This comment has been minimized.

Copy link
Member

headius commented Oct 11, 2018

@kares I went ahead with this because I want to change the invokePublic to use a call site and the rest looks fine.

@headius

This comment has been minimized.

Copy link
Member

headius commented Oct 11, 2018

@kares Nevermind that last bit...I see invokePublic has somewhat different behavior than a non-functional call site. Leaving it in place as is 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.