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] Rbx 2.5.6 upgrade #3064

Merged
merged 22 commits into from
Jun 19, 2015
Merged

[Truffle] Rbx 2.5.6 upgrade #3064

merged 22 commits into from
Jun 19, 2015

Conversation

nirvdrum
Copy link
Contributor

There's quite a bit here. A lot of it is mechanical, but to summarize:

  • Bootstrap files are largely kept the same they were
  • Core files are 100% imports from Rubinius
  • api/shims now provide method overrides for changes we used to make in place in the core files; api/shims require order has been altered suitably to make sure methods are overridden
  • Any core changes that can't be overridden are wrapped with a Truffle.omit call with a reason why; this call is replaced with a literal nil at runtime
  • BodyTranslator and ModuleTranslator were updated to provide parse time rewrites for some things we used to do manually
  • Parse warnings are disabled in the lexer & parser rather than through source modifications
  • Miscellaneous clean-ups along the way

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.

nirvdrum added 19 commits June 15, 2015 14:18
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
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove debug puts?

Copy link
Contributor Author

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 :-/

@chrisseaton
Copy link
Contributor

Don't forget [Truffle] in issues and PRs.

@chrisseaton chrisseaton added this to the truffle-dev milestone Jun 18, 2015
# diffs when upgrading Rubinius core files.
def self.omit(reason)
end
end
Copy link
Contributor

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?

Copy link
Contributor Author

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:

  1. Delineating stuff Rubinius has commented out from stuff we have commented out.
  2. 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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@chrisseaton
Copy link
Contributor

Looks good

@nirvdrum nirvdrum changed the title Rbx 2.5.6 upgrade [Truffle] Rbx 2.5.6 upgrade Jun 19, 2015
nirvdrum added a commit that referenced this pull request Jun 19, 2015
@nirvdrum nirvdrum merged commit 0dd0457 into master Jun 19, 2015
@nirvdrum nirvdrum deleted the rbx-2.5.6-upgrade branch June 19, 2015 02:39
@nirvdrum
Copy link
Contributor Author

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());
}
Copy link
Member

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).

@eregon
Copy link
Member

eregon commented Jun 19, 2015

Looks good, thank you for the hard work!

It would be nice to have some simple stats on parse time for core:/. So we could see how these kind of changes affect it (Should be fairly minimal in this case but still interesting).

@enebo enebo added this to the Non-Release 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