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

[Truffle] Use to_enum snippets to provide enumerator size blocks #3979

Merged
merged 4 commits into from
Jun 28, 2016

Conversation

bjfish
Copy link
Contributor

@bjfish bjfish commented Jun 26, 2016

@chrisseaton I've tried a few ways to do this. How does this look?

@nirvdrum
Copy link
Contributor

I don't want to get nit-picky about style, but can you please convert your tabs to spaces? Almost the entire JRuby codebase uses spaces.

@bjfish
Copy link
Contributor Author

bjfish commented Jun 26, 2016

@nirvdrum Fixed looks like my IDE was misconfigured, thanks for reminding me.

@@ -0,0 +1,51 @@
/*
* Copyright (c) 2013, 2016 Oracle and/or its affiliates. All rights reserved. This
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is all new code, so start at 2016.

@chrisseaton
Copy link
Contributor

Seems a little complex, but I guess you are trying to solve a complex problem here. The declarative aspect is good.

@chrisseaton chrisseaton added this to the truffle-dev milestone Jun 26, 2016

public static final String HASH_EACH_TO_ENUM = "to_enum(:each) { size }";

public static final String RANGE_EACH_TO_ENUM = "to_enum { size }";
Copy link
Member

Choose a reason for hiding this comment

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

For consistency, maybe always provide the name parameter?

@eregon
Copy link
Member

eregon commented Jun 27, 2016

This looks good!
Maybe we can enjoy further the fact that all snippets are very similar, by only giving it the method to compute the "enumerator size". We can then get the method name from the frame.
So, something like:
@CoreMethod(names = "each_with_index", needsBlock = true, enumeratorSize = "size")

if (method.returnsEnumeratorIfNoBlock()) {
if(!method.snippetIfNoBlock().isEmpty()) {
sequence = new SnippetIfNoBlockNode(method.snippetIfNoBlock(), sequence);
} else if (method.returnsEnumeratorIfNoBlock()) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add an assert so that we make sure both options are no given?

@bjfish
Copy link
Contributor Author

bjfish commented Jun 27, 2016

@eregon I like this idea enumeratorSize = "size" do you know how I can create Block from a string in Java. I think I've been able to create a proc like this before but not a block yet.

@bjfish
Copy link
Contributor Author

bjfish commented Jun 27, 2016

@eregon Please disregard the previous question about creating a block from a string. I think I've figured this out.

@bjfish
Copy link
Contributor Author

bjfish commented Jun 27, 2016

I've added a commit that refactors to @eregon 's suggestions

public EnumeratorSizeNode(String enumeratorSize, String methodName, RubyNode method) {
super(method.getContext(), method.getEncapsulatingSourceSection());
this.method = method;
this.snippet = "to_enum(:" + methodName + ") { " + enumeratorSize + " }";
Copy link
Member

Choose a reason for hiding this comment

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

This is the easiest way indeed :)
I thought about a CallDispatchNode since this is just a call, but building the block is just more trouble.

@eregon
Copy link
Member

eregon commented Jun 28, 2016

Let's get this merged!

@eregon eregon merged commit a222bf9 into master Jun 28, 2016
@eregon eregon deleted the truffle-to-enum-sizes branch June 28, 2016 14:17
@enebo enebo added this to the Invalid or Duplicate milestone Dec 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants