-
-
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] Refactor source section abstractions #4406
Conversation
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); |
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 intentional?
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.e. does warn do + 1
or so?
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.
Factoring bug. Fixed.
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.
Looks good.
Reviewing commit by commit helps a lot.
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 |
Yes I think eventually I'll have |
Nothing seems to fail either way so we obviously aren't testing these warnings. |
There are multiple bugs currently with warnings. o = Object.new
[1,2].each { |o| } |
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.
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.
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. |
…tions - it knows the file itself.
…to ask nodes where they came from.
… source section straight away.
046e734
to
1b31760
Compare
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 useTempSourceSection
, which is a start and end line, which is what we actually store in aRubyBaseNode
. 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.