-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
Conversation
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. |
@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 |
There was a problem hiding this comment.
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.
Seems a little complex, but I guess you are trying to solve a complex problem here. The declarative aspect is good. |
|
||
public static final String HASH_EACH_TO_ENUM = "to_enum(:each) { size }"; | ||
|
||
public static final String RANGE_EACH_TO_ENUM = "to_enum { size }"; |
There was a problem hiding this comment.
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?
This looks good! |
if (method.returnsEnumeratorIfNoBlock()) { | ||
if(!method.snippetIfNoBlock().isEmpty()) { | ||
sequence = new SnippetIfNoBlockNode(method.snippetIfNoBlock(), sequence); | ||
} else if (method.returnsEnumeratorIfNoBlock()) { |
There was a problem hiding this comment.
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?
@eregon I like this idea |
@eregon Please disregard the previous question about creating a block from a string. I think I've figured this out. |
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 + " }"; |
There was a problem hiding this comment.
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.
Let's get this merged! |
@chrisseaton I've tried a few ways to do this. How does this look?