-
-
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 sprintf parser/builder, update format nodes #4017
Conversation
return shortenInfinityString.getBytes(StandardCharsets.US_ASCII); | ||
} | ||
private String getInfiniteFormatString(final int width){ | ||
final StringBuilder inifiniteFormatBuilder = new StringBuilder(); |
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.
inifinite?
So this essentially the Antlr parser with a hand-written one? |
@eregon It was getting difficult to match the MRI behavior with the ANTLR parsing, especially its behaviors for parsing errors. I think it might be possible to get the same behavior with an ANTLR parser/listener but I think it would take a lot more effort to get it correct compared to this new parsing. |
return bytes; | ||
} | ||
|
||
|
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.
No double blank lines, please.
It's a shame we can't get Antlr to do what we want. I did struggle to get the error handling behaviour as good as it was before. I found I was adding lots of parse rules that could then be matched as the required error. Of course, passing specs trumps almost all else, so we can go with this instead. I don't think I'm going to try writing the Ruby parser in Antlr that I was planning. |
this.hasMinusFlag = hasMinusFlag; | ||
} | ||
|
||
// @TruffleBoundary |
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 try not to leave commented code like this in. At least it should be explained why it is currently commented.
Are you still iterating on this? I can't quite understand it all so I might check it out and browse it there if you're done. Some of the code needs a bit of an aesthetic tidy up (random extra blank lines and things). |
@chrisseaton I'm going to make all the recommended code review updates here to hopefully get it into good shape to merge. I can ping you after this update. Then, I need to give the |
This code was never fast-path was it? We always had the boundary in there? We should add some micro benchmarks (ask me how if you don't know) and then check we are at least the same speed as JRuby. |
@eregon @chrisseaton I've added another commit which addresses the code review comments. Thanks for the feedback. I'm not sure if the FormatIntegerNode specialization was fast path. I can look into adding back caching for this. |
|
||
@TruffleBoundary | ||
private String doFormat(final String charString, final int width, final boolean leftJustified) { | ||
return String.format("%" + (leftJustified ? "-" : "") + width + "." + width + "s", charString); |
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 just cache this generated format string? Doing that is trivial and should be an easy performance win until we implement our own formatting code.
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.
@chrisseaton I've added a commit to cache this format string
Is this ready to merge then? |
@chrisseaton Yes, I think it's ready to merge. |
I've updated to a new parser to match the spec more closely. Also, did some updates to format nodes.
I still need to continue implementing more formatting here but looking for feedback now.