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.

Sign up for GitHub

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

Jump to bottom

Extend SetU #4262

Closed
wants to merge 6 commits into from
Closed

Extend SetU #4262

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7e01d18
Extend SetU
Rot127 Feb 18, 2024
06672cc
Remove duplicate
Rot127 Apr 19, 2024
edb65be
Move set size check into if condition
Rot127 Apr 19, 2024
5d1e330
Add doxygen and some checks for advance_set_u_iter
Rot127 Apr 20, 2024
1782225
Add set_u_foreach to ForEachMacros
Rot127 Apr 21, 2024
a2a0c6c
Move set_u_iter_reset logic into for loop.
Rot127 Apr 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .clang-format
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ AlignConsecutiveMacros: true
AlignTrailingComments: false
AlignOperands: false
Cpp11BracedListStyle: false
ForEachMacros: ['rz_list_foreach', 'rz_list_foreach_safe', 'rz_pvector_foreach', 'rz_rbtree_foreach', 'rz_interval_tree_foreach', 'ls_foreach', 'rz_skiplist_foreach', 'graph_foreach_anode']
ForEachMacros: ['rz_list_foreach', 'rz_list_foreach_safe', 'rz_pvector_foreach', 'rz_rbtree_foreach', 'rz_interval_tree_foreach', 'ls_foreach', 'rz_skiplist_foreach', 'graph_foreach_anode', 'set_u_foreach']
SortIncludes: false
RequiresClausePosition: SingleLine
TypenameMacros: ['HT_', 'Ht_', 'HtName_']
17 changes: 17 additions & 0 deletions librz/include/rz_util/set.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,27 @@ typedef HtUP SetU;

RZ_API RZ_OWN SetU *set_u_new(void);
RZ_API void set_u_add(RZ_NONNULL SetU *set, ut64 u);
RZ_API ut64 set_u_size(SetU *s);
RZ_API bool set_u_contains(RZ_NONNULL SetU *set, ut64 u);
RZ_API void set_u_delete(RZ_NONNULL SetU *set, ut64 u);
RZ_API void set_u_free(RZ_NULLABLE SetU *set);

typedef struct {
st64 ti; ///< Table index
ut64 bi; ///< Bucket index
ut64 v; ///< Current value of the iteration
} SetUIter;

RZ_API void advance_set_u_iter(SetU *s, SetUIter *it);

/**
* The adcvance_set_u_iter() sets iter.ti always to the entry of the next table to check.
Rot127 marked this conversation as resolved.
Show resolved Hide resolved
* So our condition checks if ti <= set->size.
*/
#define set_u_foreach(set, iter) \
Rot127 marked this conversation as resolved.
Show resolved Hide resolved
if (set && set_u_size(set) > 0) \
for (iter.ti = 0, iter.bi = 0, iter.v = 0, advance_set_u_iter(set, &iter); iter.ti <= set->size; advance_set_u_iter(set, &iter))

#ifdef __cplusplus
}
#endif
Expand Down
35 changes: 35 additions & 0 deletions librz/util/set.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,14 @@ RZ_API void set_u_add(RZ_NONNULL SetU *set, ut64 u) {
ht_up_insert(set, u, (void *)1);
}

/**
* \brief Get the size of set \s.
*/
RZ_API ut64 set_u_size(SetU *s) {
rz_return_val_if_fail(s, 0);
return s->count;
}

/**
* \brief Check if hash set \p set contains element \p u.
*/
Expand All @@ -75,3 +83,30 @@ RZ_API void set_u_delete(RZ_NONNULL SetU *set, ut64 u) {
RZ_API void set_u_free(RZ_NULLABLE SetU *set) {
ht_up_free((HtUP *)set);
}

/**
* \brief Advances an SetUIter to the next element in
* the set \p s and saves it in \p it->v.
*
* \param s The set to iterate over.
* \param it The iterator over the set.
*/
RZ_API void advance_set_u_iter(SetU *s, SetUIter *it) {
Rot127 marked this conversation as resolved.
Show resolved Hide resolved
rz_return_if_fail(s && it);
if (it->ti >= s->size) {
it->ti++;
return;
}
for (; it->ti < s->size; it->ti++) {
if (s->table[it->ti].count == 0) {
continue;
}
if (it->bi < s->table[it->ti].count) {
it->v = s->table[it->ti].arr[it->bi].key;
it->bi++;
return;
}
it->bi = 0;
}
it->ti++;
}
70 changes: 70 additions & 0 deletions test/unit/test_util.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// SPDX-License-Identifier: LGPL-3.0-only

#include <rz_util.h>
#include <rz_util/set.h>
#include "minunit.h"

bool test_file_slurp(void) {
Expand Down Expand Up @@ -62,7 +63,76 @@ bool test_leading_zeros(void) {
mu_end;
}

bool test_set_u(void) {
SetU *set_u = set_u_new();
set_u_add(set_u, 0x5050505);
set_u_add(set_u, 0x5050505);
set_u_add(set_u, 0x6060606);
set_u_add(set_u, 0x7070707);
set_u_add(set_u, 0x7070707);
mu_assert_eq(set_u_size(set_u), 3, "Length wrong.");
mu_assert_true(set_u_contains(set_u, 0x5050505), "Value was not added.");
mu_assert_true(set_u_contains(set_u, 0x6060606), "Value was not added.");
mu_assert_true(set_u_contains(set_u, 0x7070707), "Value was not added.");

set_u_delete(set_u, 0x7070707);
mu_assert_false(set_u_contains(set_u, 0x7070707), "Value was not deleted.");
mu_assert_eq(set_u_size(set_u), 2, "Length wrong.");

// Double delete
set_u_delete(set_u, 0x7070707);
mu_assert_eq(set_u_size(set_u), 2, "Length wrong.");

size_t x = 0;
SetUIter it;
set_u_foreach (set_u, it) {
x++;
bool matches = it.v == 0x5050505 || it.v == 0x6060606;
mu_assert_true(matches, "Set contained ill-formed value.");
}
Comment on lines +87 to +92
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer rz_list_foreach like variant with separate value variable:

Suggested change
SetUIter it;
set_u_foreach (set_u, it) {
x++;
bool matches = it.v == 0x5050505 || it.v == 0x6060606;
mu_assert_true(matches, "Set contained ill-formed value.");
}
SetUIter it;
ut64 val;
set_u_foreach (set_u, it, val) {
x++;
bool matches = val == 0x5050505 || val == 0x6060606;
mu_assert_true(matches, "Set contained ill-formed value.");
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@ret2libc your opinion?

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't really have strong feelings about it. Separated variable means the type is more explicit, which is probably worth the trade against the extra line. But as long as we have a foreach without a callback, I am happy.

Copy link
Contributor

Choose a reason for hiding this comment

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

rizin/librz/include/rz_list.h

Line 35 in ea460e5

for (it = list->head; it && (pos = it->elem, 1); it = it->next)

Adapted for your case: (val = iter.v, 1)

Copy link
Member

@XVilka XVilka Apr 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think the one with the explicit value is better.

Copy link
Member

Choose a reason for hiding this comment

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

It will be more important for the SetP, where we have a room for type ambiguity.

Copy link
Member

Choose a reason for hiding this comment

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

i think the value should always be a pointer. so you can read and write the value.

Copy link
Contributor

Choose a reason for hiding this comment

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

i think the value should always be a pointer. so you can read and write the value.

For HT, yes. But in case of sets value is a key.

mu_assert_eq(x, 2, "Foreach hasn't iterated the correct number of times.");

set_u_delete(set_u, 0x6060606);
mu_assert_eq(set_u_size(set_u), 1, "Length wrong.");
set_u_delete(set_u, 0x5050505);
mu_assert_eq(set_u_size(set_u), 0, "Length wrong.");

set_u_foreach (set_u, it) {
mu_assert("Should not be reached.", false);
}
set_u_add(set_u, 0x53e0);
set_u_add(set_u, 0x53bc);
x = 0;
set_u_foreach (set_u, it) {
x++;
}
mu_assert_eq(x, 2, "Foreach hasn't iterated the correct number of times.");
set_u_delete(set_u, 0x53e0);
set_u_delete(set_u, 0x53bc);

set_u_add(set_u, 0);
set_u_add(set_u, 1);
set_u_add(set_u, 2);
set_u_add(set_u, 3);

// Add an address as key which is far away from the heap addresses.
set_u_add(set_u, 100000000);
mu_assert_true(set_u_contains(set_u, 100000000), "Not contained.");
mu_assert_eq(set_u->count, 5, "count");
mu_assert_false(set_u_contains(set_u, 6), "should not be here.");

x = 0;
set_u_foreach (set_u, it) {
x++;
}
mu_assert_eq(x, 5, "Foreach hasn't iterated the correct number of times.");

set_u_free(set_u);
mu_end;
}

int all_tests() {
mu_run_test(test_set_u);
mu_run_test(test_file_slurp);
mu_run_test(test_leading_zeros);
return tests_passed != tests_run;
Expand Down
Loading