Extend SetU

[Rot127]

Your checklist for this pull request

• I've read the guidelines for contributing to this repository
• I made sure to follow the project's coding style
• I've documented or updated the documentation of every function and struct this PR changes. If not so I've explained why.
• I've added tests that prove my fix is effective or that my feature works (if possible)
• I've updated the rizin book with the relevant information (if needed)

Detailed description

Did not add the doxygen yet. Wanted to get some feedback first.

• Add a getter for length of SetU.
• Add a foreach macro.

Test plan

Added

Closing issues

...

[Rot127]

How about we change this PR to re-implement the sets properly? SetU works fine, if it uses HtUU (not in this implementation thoug, push some commits later). And we can implement SetP by simply using SetU but casting the pointers to ut64 before.

[Rot127]

Also I couldn't find any unit tests specifically for the hash tables. Which should be definitely added.

[ret2libc]

Also I couldn't find any unit tests specifically for the hash tables. Which should be definitely added.

There are some in test/unit/test_sdb_hash.c (that's becase Ht was originally in SDB, then we moved the code to the main rizin repo but some stuff are still tied to SDB :( )

[Rot127]

@ret2libc Any opinion on the foreach macro construction? I found the implemented foreach loop via callbacks really unpractical. But It looks a little wonky. But if you in general approve it, we could have something like this for hash tables as well.

[ret2libc]

I like the approach with the iter if it works well ;)
Before using it here, though, i'd like to have it in the hashtable as well so we can really test if it works well (with ASAN).

[pelijah]

Ideally foreach macro should be implemented for HT and be adapted for sets then. But it might be a tough task to implement universal macro considering custom HTs like SdbHt. So for now it's ok to have foreach for sets only. And yet we also need the same for SetS.

[pelijah]

foreach and size method for SetS

[pelijah]

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.");
	}

[pelijah]

@ret2libc your opinion?

[Rot127]

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.

[pelijah]

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)

[XVilka]

I think the one with the explicit value is better.

[XVilka]

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

[wargio]

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

[pelijah]

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.

[Rot127]

Then let me extend the PR to include more foreach macros:

So the todo list would be:

• Use @pelijah suggested changes
• Add foreach for:
  • HtUU (and in extend SetU)
  • HtUS (and in extend SetS)
  • HtSS, HtSU
  • Testing
• Value should be always be a pointer, so writes are possible.

[pelijah]

If you are going to implement foreach for HTs, it would be better to rename ht_xx_foreach APIs to avoid code refactoring.
Here an idea of iterator type for ht_inc.h based on yours SetU iterator:

typedef struct Ht_(iter_t) {
	st64 ti; ///< Table index
	ut64 bi; ///< Bucket index
	HT_(Kv) *kv; ///< Current KV-pair.  Stop iteration if kv is NULL. 
} HT(Iter);

Also you need to provide in ht_inc.c some sort of Ht_(iter_init) and Ht_(iter_next) so you can just use the expression below in foreach macros:

for (Ht_(iter_init)(&iter); iter.kv != NULL && ... ; Ht_(iter_next)(&iter))

[Rot127]

Implemented by #4500