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

Allow sending messages to registered process with ! #1047

Merged
merged 1 commit into from
Feb 22, 2024

Conversation

UncleGrumpy
Copy link
Collaborator

@UncleGrumpy UncleGrumpy commented Feb 17, 2024

Enables sending messages to registered processes using the shorthand ! operator. If a message is sent to an atom name that is not registered using the ! operator a badarg run-time error occurs, matching OTP behaviour.

Closes #1011
Closes #98

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

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 added a few comments, anyway I think we should really add a test case.

src/libAtomVM/opcodesswitch.h Show resolved Hide resolved
@@ -2213,7 +2213,16 @@ HOT_FUNC int scheduler_entry_point(GlobalContext *glb)
#endif

#ifdef IMPL_EXECUTE_LOOP
int local_process_id = term_to_local_process_id(x_regs[0]);
term process_term = x_regs[0];
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would call it just recipient or with any name that is not biased towards a process.

int local_process_id = term_to_local_process_id(x_regs[0]);
term process_term = x_regs[0];
int local_process_id;
if (term_is_atom(process_term)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should check if recipient is a pid first, after that if it is an atom and registered, and as last thing just raise badarg.

@UncleGrumpy UncleGrumpy force-pushed the send_registered branch 5 times, most recently from aa82dfc to bb49051 Compare February 19, 2024 19:44
Copy link
Collaborator

@fadushin fadushin left a comment

Choose a reason for hiding this comment

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

LGTM

local_process_id = term_to_local_process_id(recipient_term);
} else if (term_is_atom(recipient_term)) {
local_process_id = globalcontext_get_registered_process(ctx->global, term_to_atom_index(recipient_term));
if (UNLIKELY(local_process_id == 0 )) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

leftover space here: if (UNLIKELY(local_process_id == 0 )) { -> if (UNLIKELY(local_process_id == 0)) {.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed.

CHANGELOG.md Outdated
@@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Fixed

- ESP32: fix i2c_driver_acquire and i2c_driver_release functions, that were working only once.
- Sending messages to registered processes using the `!` operator now works.
- Fixed bug in `OP_SEND` that would accept sending a message to any integer.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Te situation was a bit worse: we did allow send to any term without raising any error, and for integers was even working.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I updated the explanation of the bug.

Enables sending messages to registered processes using the shorhand `!`
operator. If a message is sent to an atom name that is not registered using the
`!` operator a `badarg` run-time error occurs, matching OTP behaviour.

closes atomvm#1011
closes atomvm#98

Signed-off-by: Winford <winford@object.stream>
@bettio bettio merged commit e7ac8fc into atomvm:release-0.6 Feb 22, 2024
84 of 85 checks passed
@UncleGrumpy UncleGrumpy deleted the send_registered branch April 15, 2024 19:42
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