feat: implement ets:lookup_elements/3#11207
Conversation
CT Test Results 3 files 136 suites 50m 0s ⏱️ Results for commit 50672d6. ♻️ This comment has been updated with latest results. To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass. See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally. Artifacts// Erlang/OTP Github Action Bot |
9cb12d2 to
bd7cf99
Compare
c92524c to
50672d6
Compare
jhogberg
left a comment
There was a problem hiding this comment.
Thanks for the PR! I've taken a brief look and have a few comments.
| CHECK_TABLES(); | ||
|
|
||
| DB_BIF_GET_TABLE(tb, DB_READ, LCK_READ, BIF_ets_lookup_elements_3); | ||
|
|
||
| if (!is_tuple(BIF_ARG_3) || (tp = tuple_val(BIF_ARG_3), | ||
| index_cnt = arityval(tp[0]), | ||
| index_cnt < 0)) { | ||
| db_unlock(tb, LCK_READ); | ||
| BIF_ERROR(BIF_P, BADARG); | ||
| } | ||
|
|
||
| if(index_cnt==0) { return TUPLE0; } | ||
|
|
||
| for(int i=0; i<index_cnt; i++) { | ||
| if (is_not_small(tp[i+1]) || (signed_val(tp[i+1]) < 1)) { | ||
| db_unlock(tb, LCK_READ); | ||
| BIF_ERROR(BIF_P, BADARG); | ||
| } | ||
| } |
There was a problem hiding this comment.
Checking for errors before locking the table is a bit less error prone (cf. if(index_cnt==0) { return TUPLE0; } not unlocking):
| CHECK_TABLES(); | |
| DB_BIF_GET_TABLE(tb, DB_READ, LCK_READ, BIF_ets_lookup_elements_3); | |
| if (!is_tuple(BIF_ARG_3) || (tp = tuple_val(BIF_ARG_3), | |
| index_cnt = arityval(tp[0]), | |
| index_cnt < 0)) { | |
| db_unlock(tb, LCK_READ); | |
| BIF_ERROR(BIF_P, BADARG); | |
| } | |
| if(index_cnt==0) { return TUPLE0; } | |
| for(int i=0; i<index_cnt; i++) { | |
| if (is_not_small(tp[i+1]) || (signed_val(tp[i+1]) < 1)) { | |
| db_unlock(tb, LCK_READ); | |
| BIF_ERROR(BIF_P, BADARG); | |
| } | |
| } | |
| CHECK_TABLES(); | |
| if (!is_tuple(BIF_ARG_3)) { | |
| BIF_ERROR(BIF_P, BADARG); | |
| } | |
| tp = tuple_val(BIF_ARG_3); | |
| index_cnt = arityval(tp[0]); | |
| if (index_cnt < 0) { | |
| BIF_RET(TUPLE0); | |
| } | |
| for (int i = 1; i <= index_cnt; i++) { | |
| if (is_not_small(tp[i]) || (signed_val(tp[i]) < 1)) { | |
| BIF_ERROR(BIF_P, BADARG); | |
| } | |
| } | |
| DB_BIF_GET_TABLE(tb, DB_READ, LCK_READ, BIF_ets_lookup_elements_3); |
| *hpp += num_positions + 1; | ||
| for (i = 0; i < num_positions; i++) { | ||
| Eterm *hp; | ||
| tp[i+1] = db_copy_element_from_ets(tb, |
There was a problem hiding this comment.
This will result in an allocation for each non-trivial element we've referenced. It would be better to follow what db_copy_element_from_ets does here and use a single allocation for everything (including the top-level tuple).
If
positionsis an empty tuple, an empty tuple is returned without going further into the process.I tried to replicate existing code patterns from
lookup_element/3for all versions ofetstables.IndexesandPositionsare synonyms in code, but again - I tried to match existing naming per-file.My BEAM-memory-management skills are pretty low so please check that part twice.
Testing is a bit scarce, but
lookup_element/3is also not tested that much directly.I could also implement
lookup_elements/4if needed, but just wanted this to get some review before going into that.Ping @RobinMorisset , @ansd and @jhogberg
Closes #10211