Support constant string concatenation #3318
Conversation
compiler-core/src/erlang.rs
Outdated
|
|
||
| Constant::StringConcatenation { .. } => docvec![const_inline(value, env), "/binary"], | ||
|
|
||
| _ => panic!("Constant string concatenation argument can only be a string or const var at code generation time"), |
There was a problem hiding this comment.
Please remove these panics, code generation is not the place to do type checking, and other variants could be suitable in future. Calling const_inline on whatever the Constant::* is is good.
| r#" | ||
| const foo = "foo" | ||
| const foobar = foo <> "bar" | ||
| const foofoobarbaz = foo <> foobar <> "baz" |
There was a problem hiding this comment.
Split this into multiple tests please, we only test one example per test.
Could we also have a test for a much larger number of concatenations in one constant too please.
DeviousStoat
force-pushed
the
const-string-concatenation
branch
2 times, most recently
from
53dce10
to
f978801
Compare
|
I think I addressed your comments. |
|
|
||
| -spec main() -> binary(). | ||
| main() -> | ||
| <<"a"/utf8, |
There was a problem hiding this comment.
This should render like this:
main() ->
<<"a"/utf8,
"b"/utf8,
"c"/utf8,
...There was a problem hiding this comment.
ooh right, it's not just format problem, we don't need to /binary the concatenation, we can build a bit array of /utf8 strings. This is a lot nicer now, I am starting to understand what you meant with "string concatenation doesn't require compile time code execution"
compiler-core/src/parse/tests.rs
Outdated
| fn const_string_concat_naked_right() { | ||
| assert_module_error!( | ||
| " | ||
| const foo = \"foo\" <> |
There was a problem hiding this comment.
Remove the foobar from this file please 🙏
There was a problem hiding this comment.
oops sorry missed this one
DeviousStoat
force-pushed
the
const-string-concatenation
branch
from
5cc82df
to
5e681e5
Compare
compiler-core/src/erlang.rs
Outdated
| // instead of | ||
| // <<"a"/utf8, | ||
| // "b"/utf8>> | ||
| .set_nesting(INDENT + 2) |
There was a problem hiding this comment.
This will be wrong if the indent level is supposed to be anything other than 1!
For example:
const x = [
<<
...,
...,
>>
]I don't mind so much about the exact format used, but the indentation should increase as there's more nesting
There was a problem hiding this comment.
oh right, I pushed a change with a test case for a long concatenation in a list.
Let me know what you think, it doesn't exactly has the format that you specified before but the indentation should be good now.
I think to render like that:
main() ->
<<"a"/utf8,
"b"/utf8,
"c"/utf8,
...would require changes in the nesting logic or a special case I am not sure, I could look into it if you want
Implements support for string concatenation in module constants' expressions.
closes #3208