Auto infer size of matched variable in bitstrings #13106
Conversation
sabiwara
force-pushed
the
auto-byte-size
branch
from
9ea5085
to
4f0dda6
Compare
lib/elixir/src/elixir_bitstring.erl
Outdated
| EAcc = concat_or_prepend_bitstring(Meta, ELeft, ERight, Acc, ES, MatchOrRequireSize), | ||
| expand(BitstrMeta, Fun, T, EAcc, {SS, OriginalS}, ES, alignment(Alignment, EAlignment), RequireSize); | ||
| case {ELeft, Right} of | ||
| {{'^', _, [{var, _, _}]}, {Type, _, Atom}} when Type == binary orelse Type == bitstring, is_atom(Atom) -> |
There was a problem hiding this comment.
We should not match on Right. Macros have not been expanded nor we have normalized bytes and bits to binary/bitstring. We may need to move this logic deep inside expand_specs.
There was a problem hiding this comment.
You're right, sorry for that. Will also include tests for these.
My first attempt was deep inside expand_specs, and then went for this simplified version because it was getting unwieldy 😅 Will go back to it.
There was a problem hiding this comment.
There is a MatchOrRequireSize argument already. Maybe instead of passing it in, have the expand_specs return ok or size_missing. Then you can decide here to either raise, skip or match on pin?
There was a problem hiding this comment.
I'm not sure if this is what you had in mind but my approach used the MatchOrRequireSize: 7882446
| @@ -269,6 +269,16 @@ defmodule Kernel.BinaryTest do | |||
| assert <<1::size((^foo).bar)>> = <<1::5>> | |||
| end | |||
|
|
|||
| test "automatic size computation of matched bitsyntax variable" do | |||
| var = "foo" | |||
| <<^var::binary, rest::binary>> = "foobar" | |||
There was a problem hiding this comment.
it would be really nice if this worked too:
^var <> rest = "foobar"There was a problem hiding this comment.
It will work, <> expands to the above.
There was a problem hiding this comment.
Needs to be addressed though, right now it fails with
** (ArgumentError) the left argument of <> operator inside a match should always be a literal binary because its size can't be verified. Got: ^var
There was a problem hiding this comment.
Ah, we probably explicit forbid it for better error messages.
There was a problem hiding this comment.
Will do it as a follow up PR, or do you prefer me to do it here directly?
There was a problem hiding this comment.
Let's do it here, I think it will be mostly removing code.
There was a problem hiding this comment.
It was indeed 🙂
ff24c64
Close #13089