Add ets:delete/1, ets:delete refactor #1461
Conversation
TheSobkiewicz
force-pushed
the
thesobkiewicz/nifs/ets/refactor_delete
branch
3 times, most recently
from
1a801a2 to
c91b514
Compare
TheSobkiewicz
force-pushed
the
thesobkiewicz/nifs/ets/refactor_delete
branch
5 times, most recently
from
d657d7a to
c8b5a2c
Compare
| SMP_UNLOCK(ets_table); | ||
| list_remove(&ets_table->head); | ||
| ets_table_destroy(ets_table, ctx->global); |
There was a problem hiding this comment.
So, what happens if we unlock here and somebody locks it for other operation when we destroy it in the meantime?
There was a problem hiding this comment.
Not sure how to deal with it, without SMP_UNLOCK it results in abort
| fun() -> ets:insert(Tid, {gnu, gnat}) end | ||
| ), | ||
| Ntid = ets:new(test_delete_table, []), | ||
| true = ets:delete(Ntid), |
There was a problem hiding this comment.
We should also test what happens both when deleting something that has been already deleted, and when deleting twice the same thing.
There was a problem hiding this comment.
We also don't test for deleting non-existent table. Additionally, I don't get why we're doing insert + lookup if we don't do anything with it.
TheSobkiewicz
force-pushed
the
thesobkiewicz/nifs/ets/refactor_delete
branch
from
c8b5a2c to
152632a
Compare
libs/estdlib/src/ets.erl
Outdated
| @@ -141,3 +142,12 @@ update_counter(_Table, _Key, _Params) -> | |||
| ) -> integer(). | |||
| update_counter(_Table, _Key, _Params, _Default) -> | |||
| erlang:nif_error(undefined). | |||
| %%----------------------------------------------------------------------------- | |||
| %% @param Table a reference to the ets table | |||
| %% @returns true; otherwise, an error is raised if arguments are bad | |||
There was a problem hiding this comment.
Nit: I don't find this @returns much of an help to be honest.
src/libAtomVM/ets.c
Outdated
| return EtsPermissionDenied; | ||
| } | ||
|
|
||
| struct ListHead *_ets_tables_list = synclist_wrlock(&ctx->global->ets.ets_tables); |
There was a problem hiding this comment.
Nit: Why it starts with underscore?
| fun() -> ets:insert(Tid, {gnu, gnat}) end | ||
| ), | ||
| Ntid = ets:new(test_delete_table, []), | ||
| true = ets:delete(Ntid), |
There was a problem hiding this comment.
We also don't test for deleting non-existent table. Additionally, I don't get why we're doing insert + lookup if we don't do anything with it.
TheSobkiewicz
force-pushed
the
thesobkiewicz/nifs/ets/refactor_delete
branch
2 times, most recently
from
ba8a90b to
27a6254
Compare
Signed-off-by: Tomasz Sobkiewicz <[email protected]>
TheSobkiewicz
force-pushed
the
thesobkiewicz/nifs/ets/refactor_delete
branch
from
27a6254 to
5fccfb9
Compare
|
@bettio, @TheSobkiewicz maybe ready for review? |
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
Changes:
Based on Add option to insert list in ets:insert, ets:lookup refactor #1405.
Use Cases for the Helper Functions:
The new helper functions can be utilized in the following ETS operations to reduce code duplication: