Cleanup ETS implementation #1614
Conversation
Signed-off-by: Jakub Gonet <[email protected]>
Signed-off-by: Jakub Gonet <[email protected]>
Signed-off-by: Jakub Gonet <[email protected]>
Signed-off-by: Jakub Gonet <[email protected]>
Signed-off-by: Jakub Gonet <[email protected]>
Signed-off-by: Jakub Gonet <[email protected]>
There was a problem hiding this comment.
The PR is generally ok, there are few things that should be improved (that I commented).
Furthermore, if possible it would be great to have more information in the commit message.
Such as:
Update tests message is not really informative. A better commit message should answer 2 questions:
- what change are you making (that is good for the commit subject)
- why are you making that change (commit extended message)
Also it is good to add some "tag" to the commit message, such as:
Remove unused import -> ets_hashtable.c: remove unused include
(Beware, C has includes, not imports ;) )
| if (term_compare(key, node->key, TermCompareExact, global) == TermEquals) { | ||
| TermCompareResult cmp = term_compare(key, node->key, TermCompareExact, global); | ||
| if (UNLIKELY(cmp == TermCompareMemoryAllocFail)) { | ||
| return EtsHashtableError; |
There was a problem hiding this comment.
It is ok to raise a specific error about out of memory.
On microcontrollers it is not an unlikely event, so it is better to keep it clear when it happens.
| @@ -81,51 +84,37 @@ static void ets_add_table(struct Ets *ets, struct EtsTable *ets_table) | |||
| synclist_unlock(&ets->ets_tables); | |||
| } | |||
|
|
|||
| static struct EtsTable *ets_get_table_by_ref(struct Ets *ets, uint64_t ref, TableAccessType access_type) | |||
| static struct EtsTable *ets_get_table(struct Ets *ets, int64_t process_id, term name_or_ref, TableAccessType access_type) | |||
There was a problem hiding this comment.
actually process_id is an int32.
| @@ -28,6 +28,9 @@ | |||
| #include "overflow_helpers.h" | |||
| #include "term.h" | |||
|
|
|||
| #define ETS_NO_INDEX SIZE_MAX | |||
There was a problem hiding this comment.
are we using in any place ETS_NO_INDEX? I cannot find it here.
| bool found = is_atom ? table->is_named && table->name == name : table->ref_ticks == ref; | ||
| if (found) { | ||
| bool is_owner = table->owner_process_id == process_id; | ||
| bool can_read = access_type == TableAccessRead && (table->access_type != EtsAccessPrivate || is_owner); |
There was a problem hiding this comment.
I suggest adding more parenthesis for improved readability.
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