-
Notifications
You must be signed in to change notification settings - Fork 105
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
Add functions to load and dump hex string #1304
Conversation
c3ba0f5
to
cf21456
Compare
Thank you for your review, I updated my code |
I am also concerned CI didn't run because of the merge conflict, even if it LGTM. |
Everything looks good to me too. However, if you feel confident with
Also by doing so you would allow GH running again the CI. That would be super appreciated. |
Okay, I'll rebase the branch onto release-0.6 to clean up the history and to resolve conflict. |
21444d5
to
2590cb2
Compare
On The docs of |
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? |
2590cb2
to
32887ae
Compare
I had forgot that a language server I use behaves differently from erlfmt. So I fixed it. |
The project used to be LGPL then we switched to Apache-2.0 OR LGPL, so for end users means pick your favorite license. |
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.
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.
32887ae
to
219828c
Compare
I just appended @realglobe-Inc at the end of the copyright. |
@aiotter I'm ok with this, still there is a problem with buffer size: when dealing with a base like 2, for which the maximum number has more digits than the 23 used for base 10. |
@bettio Oh I already sent a reply for your comment. Didn't you missed it, or am I missing something? |
I don't know why, but my old comment is regarded as a new code review in draft. Maybe a bug of GitHub? I abandoned the code review and resent a comment, so now you can check it ;-) |
yeah, looks likely a GitHub bug, it was very odd. Anyway I left a reply comment. |
Signed-off-by: Yuto Oguchi <[email protected]>
Signed-off-by: Yuto Oguchi <[email protected]>
Signed-off-by: Yuto Oguchi <[email protected]>
219828c
to
11b23ab
Compare
I increased the buffer size. It seems ready to be merged now! |
Thanks for the changes, I'm going to merge it right now. |
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.