-
-
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] Rbx 2.5.6 upgrade #3064
Conversation
…nge implementation.
…ify corner cases.
…form::POSIX.getgroups.
Part of this upgrade was to limit the number of modifications made directly to Rubinius files. Method overrides are now done in the api/shims files where possible. Where code cannot be overridden or must otherwise be removed, the Truffle.omit utility was introduce to clearly denote these sections. The intent is to make it clearer where we can't use Rubinius directly and to make future upgrades go easier.
Conflicts: truffle/src/main/java/org/jruby/truffle/nodes/core/RangeNodes.java truffle/src/main/java/org/jruby/truffle/nodes/rubinius/PosixNodes.java truffle/src/main/java/org/jruby/truffle/runtime/core/CoreLibrary.java
@@ -122,6 +122,7 @@ def self.watch_channel | |||
|
|||
# Spin up the thread up front to avoid a thread collision problem spinning | |||
# it up lazily. | |||
puts caller |
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.
Remove debug puts?
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.
Thanks. I had removed it, then needed it again, and forgot to remove it the second time :-/
Don't forget [Truffle] in issues and PRs. |
# diffs when upgrading Rubinius core files. | ||
def self.omit(reason) | ||
end | ||
end |
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.
Is this just for documentation?
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.
Yeah. The two cases I wanted to handle are:
- Delineating stuff Rubinius has commented out from stuff we have commented out.
- Annotating things we've essentially deleted (just deleting them means that needs to be reevaluated each time we sync upstream).
So, it's essentially documentation for why we can't run something while keeping the diff between different Rubinius files minimal. It's also easy to grep for.
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.
Sorry no I mean what's the point of this definition - it's handled in translator so we don't need Ruby code do we?
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.
Oh. I had it there before the translator stuff was written and it seemed more Ruby-like to keep it. I can delete it though.
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.
Re: "more Ruby-like" ... we could just have our own top-level methods that look more like keywords. I figured this approach looked more like code written in Ruby and handling it in the translator was an optimization pass.
Looks good |
…ms like we do for others.
I merged, but I'm still open to feedback. |
// We're never going to run the omitted code and it's never used as the RHS for anything, so just | ||
// replace the call with nil. | ||
return new LiteralNode(context, sourceSection, context.getCoreLibrary().getNilObject()); | ||
} |
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.
We do need to extract some local vars in the Translator, it becomes really hard to read (general comment, not about this particular case).
Looks good, thank you for the hard work! It would be nice to have some simple stats on parse time for |
There's quite a bit here. A lot of it is mechanical, but to summarize:
I haven't yet untagged any specs this would make passing. I'll do that after the merge if you guys don't have any objection.
Ping @chrisseaton, @eregon, @pitr-ch, @bjfish.