Skip to content
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

Add functions to load and dump hex string #1304

Open
wants to merge 3 commits into
base: release-0.6
Choose a base branch
from

Conversation

aiotter
Copy link

@aiotter aiotter commented Oct 2, 2024

These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).

SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later

Closes #1287.

@aiotter aiotter force-pushed the dev/binary-decode_hex branch 3 times, most recently from c3ba0f5 to cf21456 Compare October 2, 2024 13:31
src/libAtomVM/nifs.c Outdated Show resolved Hide resolved
tests/erlang_tests/test_binary_to_integer.erl Outdated Show resolved Hide resolved
@aiotter
Copy link
Author

aiotter commented Oct 3, 2024

Thank you for your review, I updated my code

@aiotter aiotter requested a review from pguyot October 3, 2024 08:11
libs/estdlib/src/erlang.erl Outdated Show resolved Hide resolved
@pguyot
Copy link
Collaborator

pguyot commented Oct 3, 2024

I am also concerned CI didn't run because of the merge conflict, even if it LGTM.

@bettio
Copy link
Collaborator

bettio commented Oct 3, 2024

Everything looks good to me too.

However, if you feel confident with git rebase -i release-0.6 or any other tool, would you mind rewriting history so:

  • Fix binary_to_int/2 to work in the same way with OTP can be fixed up in Add support for binary_to_integer/2
  • Fix binary:decode_hex/1 to work in the same way with OTP can be fixed up in Add support for binary:decode_hex/1 and binary:encode_hex/1,2

Also by doing so you would allow GH running again the CI.

That would be super appreciated.

@aiotter
Copy link
Author

aiotter commented Oct 4, 2024

Okay, I'll rebase the branch onto release-0.6 to clean up the history and to resolve conflict.

@aiotter
Copy link
Author

aiotter commented Oct 4, 2024

On libs/estdlib/src/binary.erl, I copied and pasted only the docs and specs of the functions from OTP. Specs are just a piece of information and have no copyright, but the docs can. Do you think it can cause a copyright problem?

The docs of libs/exavmlib/lib/Base.ex is also copied from Elixir, and it is sure that the Elixir contributors have copyright for them. So I made that clear at the top of the file.

@aiotter
Copy link
Author

aiotter commented Oct 4, 2024

btw, it seems every file of the source code has Apache 2.0 at the top of it. Which part of AtomVM is licensed under LGPL?

@aiotter
Copy link
Author

aiotter commented Oct 4, 2024

I had forgot that a language server I use behaves differently from erlfmt. So I fixed it.
https://github.com/atomvm/AtomVM/actions/runs/11171766326/job/31064782745?pr=1304

@bettio
Copy link
Collaborator

bettio commented Oct 4, 2024

btw, it seems every file of the source code has Apache 2.0 at the top of it. Which part of AtomVM is licensed under LGPL?

The project used to be LGPL then we switched to Apache-2.0 OR LGPL, so for end users means pick your favorite license.
It's ok to have some module and libraries that are Apache-2.0 only, while LGPL only is not allowed.

Copy link
Collaborator

@bettio bettio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I apologize for noticing this late, but there is a small detail that must be fixed: bases lower than 10 requires more room in the buffer. So we must update it accordingly to avoid potential buffer overflows.
Just make this change as a fixup, there is no need to add an additional commit.

I also suggest converting a 62 bit number to binary as an additional test case.

Everything else is good.

if (UNLIKELY((base < 2) || (base > 36))) {
RAISE_ERROR(BADARG_ATOM);
}

char null_terminated_buf[24];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Base 2 can easily lead to to a buffer overflow when picking a 32 bits number (it cannot fit into a 24 chars buffer). Let's update the buffer size.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants