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 support for formatting ports #1474

Merged
merged 2 commits into from
Jan 19, 2025

Conversation

UncleGrumpy
Copy link
Collaborator

@UncleGrumpy UncleGrumpy commented Jan 15, 2025

Add support for io_lib (and io, etc...) to format ports.
Adds port_to_list/1 to erlang module exports to keep dialyzer happy.
Adds a missing clause to handle port terms in memory_estimate_usage(term) function that is necessary for formatting ports for io or other purposes.

This is a continuation of #1473.

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

@@ -292,6 +292,8 @@ format_spw(_Format, T) when is_float(T) ->
erlang:float_to_list(T);
format_spw(_Format, T) when is_pid(T) ->
erlang:pid_to_list(T);
format_spw(_Format, T) when is_port(T) ->
erlang:port_to_list(T);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you so much.
Could we have a test for this clause that passes on both BEAM and AtomVM?

open_port({spawn, "echo"}, []) works on both platforms AFAIK.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Absolutely, I should have thought to add tests... "echo" is what I have been using to test locally ;-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you so much. Could we have a test for this clause that passes on both BEAM and AtomVM?

The short answer is no. Lol! Any test that passes on the BEAM will fail on AtomVM until #1473 is merged. But I will write one that will make sure we are returning a list that matches the format that OTP returns.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to make 1474 a continuation of 1473 :-)

@UncleGrumpy UncleGrumpy force-pushed the format_ports branch 3 times, most recently from 8ed6d86 to 68a1c78 Compare January 18, 2025 20:40
@UncleGrumpy
Copy link
Collaborator Author

Note: test_port_to_list should pass on OTP and fail on AtomVM until #1473 is merged.

@UncleGrumpy UncleGrumpy force-pushed the format_ports branch 3 times, most recently from e866bd7 to 82d6f6d Compare January 18, 2025 22:53
@UncleGrumpy UncleGrumpy force-pushed the format_ports branch 2 times, most recently from a3f282e to ef9fa14 Compare January 19, 2025 04:32
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.

Seem to fail even after merging #1473

Adds a missing clause to handle port terms in `memory_estimate_usage(term)` function that is necessary
for formatting ports for io or other purposes.

Signed-off-by: Winford <winford@object.stream>
@UncleGrumpy UncleGrumpy force-pushed the format_ports branch 2 times, most recently from 057570e to 0b270eb Compare January 19, 2025 18:33
Add support for io_lib (and io, etc...) to format ports.
Adds port_to_list/1 to erlang module exports to keep dialyzer happy.
Add test/erlang_tests/test_port_to_list.erl to test result of port_to_list/1 using the "echo" port.

Signed-off-by: Winford <winford@object.stream>
@bettio bettio merged commit 6da1e93 into atomvm:feature/distributed-erlang Jan 19, 2025
107 checks passed
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.

3 participants