-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Handle non-binary bitstring in struct default values #14363
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
Conversation
7c8409e
to
1790516
Compare
lib/elixir/test/elixir/map_test.exs
Outdated
defstruct bitstring: <<255, 127::7>> | ||
end | ||
|
||
assert struct!(WithBitstring).bitstring == <<255, 127::7>> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is not needed as even if we use an empty bin in your implementation the value is still the same. Following the issue there should be a check if the module compiles. Optionally there could be an extra test for the new :elixir_erl.elixir_to_erl/2
function clause.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is not needed as even if we use an empty bin in your implementation the value is still the same.
@Eiji7 good catch thank you! 💜 This was needed by struct_info, fixed.
Following the issue there should be a check if the module compiles.
This is being checked, otherwise defmodule
would fail.
Optionally there could be an extra test for the new :elixir_erl.elixir_to_erl/2 function clause.
I thought the same, but I don't think there is any direct test for elixir_to_erl
at this moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beautiful! And 👍 on backport!
Close #14362
For instance, the erlang AST for
<<255, 127::7>>
is:To be backported after merge.