New sprintf implementation #7612
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The goal of this new design is to make it much simpler to modify and update when we find a bug or have to add a new identifier. A secondary longer goal is to decouple processing of the format string from the generation of the final string when executing sprintf. The value of this decoupling is we can push the format parsing phase into the sprintf callsite so we only have to process the format once. In fact once we get to that point we can potentially just bytecode generate the string building piece. The current point of this work is we can completely parse the format string into a sequence of tokens. It is done sans bugs or decisions to add/change any fields based on work on the interpreter. At this point to use it you need to set the env var SPRINTF=anything to enable new runtime and it will only work with %i, %d, and %u. Future work will be to keep adding new directives but not be trying very hard to make one method process to many directives. In the case of the three (i,d,u) 'i' is an alias of 'd' and 'u' processes unsigned values a little differently when setting up the value. Any more complexity and u would have been its own method.
May need to unit test byteListUpto but for now this passes more tests.
indexed args in sprintf were still also using that index as its width.
getArg will now get named parameters.
Implement argument errors for mixed format specifiers.
The parser assumed width and arg index would occur in the same order: %*1$2$d but it can also be: %2$*1$d I also changed some naming.
I fixed another incompatibility and realized hasWidth is the right name. This commit also fixed asking for next arg if hasWidth for an unnumbered format field.
%#o with a negative values does not prefix with a '0'. Strange but true.
"%*10d" is mixing numbered '*' with 10d unnumbered.
… succ. Lots of random fixes to the sprintf parser. A grab bag of smaller fixes.
Telling between explictly supplied precision and an argument provided one is still messy but it should be working now. I will try and circle back to simplify FormatToken since the other two (index, width) are not as complex. Also removed dead parameter in processDigits.
%5.0d was zero padding and asking for an extra argument before this change.
We now pass more specs and tests with new code and we also want to see mistakes made with the new code during CI runs.
Fails two specs and one mri test Modified: core/src/main/java/org/jruby/util/SprintfParser.java
According to local tests just one test fail
This unfortunately does not mean it still does not need to check it in the case of '*' but it makes sense to make the token as known as possible.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Created to track #6913 and work from 2021 on the
new_sprintfbranch. Still in progress.