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 sprintf parser/builder, update format nodes #4017

Merged
merged 3 commits into from
Jul 18, 2016

Conversation

bjfish
Copy link
Contributor

@bjfish bjfish commented Jul 14, 2016

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.

return shortenInfinityString.getBytes(StandardCharsets.US_ASCII);
}
private String getInfiniteFormatString(final int width){
final StringBuilder inifiniteFormatBuilder = new StringBuilder();
Copy link
Member

Choose a reason for hiding this comment

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

inifinite?

@eregon
Copy link
Member

eregon commented Jul 15, 2016

So this essentially the Antlr parser with a hand-written one?
Is Antlr not flexible or practical here?
The disadvantage is the whole parsing state is mixed with validation logic, the parser logic is re-implemented, and it's probably more prone to bugs in parsing.
OTOH, it might be interesting performance-wise.

@bjfish
Copy link
Contributor Author

bjfish commented Jul 15, 2016

@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;
}


Copy link
Contributor

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.

@chrisseaton
Copy link
Contributor

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
Copy link
Contributor

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.

@chrisseaton
Copy link
Contributor

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

@bjfish
Copy link
Contributor Author

bjfish commented Jul 15, 2016

@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 Integer and Float and other nodes more attention to pass more specs, be more readable, and performant as needed.

@chrisseaton
Copy link
Contributor

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.

@bjfish
Copy link
Contributor Author

bjfish commented Jul 15, 2016

@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);
Copy link
Contributor

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.

Copy link
Contributor Author

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

@chrisseaton
Copy link
Contributor

Is this ready to merge then?

@chrisseaton chrisseaton added this to the truffle-dev milestone Jul 18, 2016
@bjfish
Copy link
Contributor Author

bjfish commented Jul 18, 2016

@chrisseaton Yes, I think it's ready to merge.

@chrisseaton chrisseaton merged commit 25768d9 into master Jul 18, 2016
@chrisseaton chrisseaton deleted the truffle-sprintf-update branch July 18, 2016 16:43
@enebo enebo added this to the Invalid or Duplicate 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