Format string on parsing - #429
Conversation
CohenArthur
left a comment
There was a problem hiding this comment.
Could you also add a test with an interpolation right at the beginning of the string? I know you handle it properly in the parser, but just for later regressions. Something like "{world} hello" or whatever.
Also, we need to remove the expansion phase from the JkString struct but we should do that in a later PR.
Thanks a lot for this!!!
| pub(crate) fn string_constant(input: &str) -> ParseResult<&str, Box<dyn Instruction>> { | ||
| let (input, string_value) = Token::string_constant(input)?; | ||
| let (input, inner) = preceded(Token::double_quote, ConstantConstruct::inner_string)(input)?; | ||
| let string = inner.unwrap_or_else(|| Box::new(JkString::from(""))); |
| if let Ok((input, _)) = Token::double_quote(input) { | ||
| Ok((input, None)) |
There was a problem hiding this comment.
So if the first character we see is a double quote, we return None? Is that why we unwrap_or_else in the caller?
There was a problem hiding this comment.
The elegant implementation here was to return an empty jkstring when we reach a double quote, that way we indiscriminately concat whatever we have afterwards instead of matching as you can see in the concat function, it would also avoid the unwrap_or_else in the function above.
However doing this cause EVERY string to call .concat("") when built, it also causes extra heap allocation in the AST to create this method call.
That's why instead i return an Option and handle it as a special case, thus avoiding unnecessary heap allocation and slightly better runtime performances.
|
Also @SanderJSA, could you change the target branch to |
Done |
This PR formats string at parse time instead of eval time,
here's an example change in behavior caused by this PR:
before:
after:
The big difference it this string is now formatted in the context where it is built instead on where it is called.
I'm not sure if/what could I should remove to disable de eval time formatting though.