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] Refactor source section abstractions #4406

Merged
merged 17 commits into from
Dec 22, 2016

Conversation

chrisseaton
Copy link
Contributor

Sorry, this is all refactoring so it won't be possible to review as normal. But hopefully this refactoring will make the review for the actual precise parser possible to read.

Removes JRuby's SourcePosition and associated interfaces, and just use TempSourceSection, which is a start and end line, which is what we actually store in a RubyBaseNode. Switch from 0 indexed lines to 1 indexed, as Truffle does.

Inline source position information so that we have less objects. TempSourceSection is just a throwaway tuple to pass them around.

The next step is actually making source information character index precise and then making that work with the Truffle API, which may be tricky as I also have to solve the byte[] / String problems.

@chrisseaton chrisseaton added this to the truffle-dev milestone Dec 21, 2016
@chrisseaton chrisseaton self-assigned this Dec 21, 2016
@chrisseaton
Copy link
Contributor Author

Oh and another big change - don't carry around the source file. We can always figure that out anyway.

@@ -188,7 +188,7 @@ public void warning(String message) {
public void warning(ID id, String fileName, int lineNumber, String message) {
if (!runtime.warningsEnabled() || !runtime.isVerbose()) return;

warn(id, fileName, lineNumber, message);
warn(id, fileName, lineNumber - 1, message);
Copy link
Member

Choose a reason for hiding this comment

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

Is this intentional?

Copy link
Member

@eregon eregon Dec 21, 2016

Choose a reason for hiding this comment

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

i.e. does warn do + 1 or so?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Factoring bug. Fixed.

Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

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

Looks good.
Reviewing commit by commit helps a lot.

@eregon
Copy link
Member

eregon commented Dec 21, 2016

You might be able to avoid TempSourceSection completely, by writing the 2 fields on the translated nodes from the ParseNode directly, like

return t(node, new NilLiteralNode());
void t(parserNode, truffleNode) {
    truffleNode.startLine = parserNode.startLine;
    truffleNode.endLine = parserNode.endLine;
}

That's similar to what I do in Mozart-Graal: https://github.com/eregon/mozart-graal/blob/master/vm/src/org/mozartoz/truffle/translator/Translator.java#L285
Nice to have, but can be done later as it requires changing some constructors taking SourceSection or TempSourceSection.

@chrisseaton
Copy link
Contributor Author

Yes I think eventually I'll have rubyNode.transferSourceSection(parseNode).

@chrisseaton
Copy link
Contributor Author

Nothing seems to fail either way so we obviously aren't testing these warnings.

@eregon
Copy link
Member

eregon commented Dec 22, 2016

There are multiple bugs currently with warnings.
I fixed the most obvious one which prevented any warning to show: 0e27962.
Line numbers were currently wrong, so I think the last commit is right.
To test, you can try something like
jt ruby -w test.rb

o = Object.new
[1,2].each { |o|  }

Copy link
Contributor

@nirvdrum nirvdrum left a comment

Choose a reason for hiding this comment

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

What's driving the rename? I don't think I follow what makes these temporary or why that needs to be codified in the name.

@chrisseaton
Copy link
Contributor Author

The rename was just on a whim - it's shouldn't be around too long (will be refactored away) so I gave it a name to keep that clear in my mind as I use it. The rename isn't the important bit. Removing all the abstraction was.

@chrisseaton chrisseaton force-pushed the truffle-precise-parser branch from 046e734 to 1b31760 Compare December 22, 2016 17:44
@eregon eregon merged commit ad3ac33 into truffle-head Dec 22, 2016
@eregon eregon deleted the truffle-precise-parser branch December 22, 2016 18:17
@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

4 participants