lib: implement hashset

[pzmarzly]

Currently, bf_set uses bf_list under the hood. This is suboptimal performance-wise. Here I am changing bf_set to be backed by bf_hashset.

• lib: helper: add FNV-1a hash function - imported from another PR, a simple hash function.
• lib: add bf_hashset - main commit. Implements a simple hashset (tombstones on deletion, grows 2x when load factor exceeds 50%, never shrinks, linear probing on collision). See https://en.wikipedia.org/wiki/Linear_probing (with "special flag value" strategy in "Deletion" section).
• lib: set: use bf_hashset for elems field - changes bf_set's inner structure

See #460 for previous attempt.

Fixes #418

[github-actions]

Claude review of PR #470 (c911d48)

Suggestions

• Defensive assert for free_idx — src/libbpfilter/core/hashset.c:284 — free_idx could remain SIZE_MAX if internal invariants are violated; add assert(free_idx < set->cap) before first use
• Deduplicate _bf_round_next_power_of_2 — src/libbpfilter/core/hashset.c:26 — duplicated with cgen/program.c:66; the two copies differ in correctness (zero handling, 64-bit support)
• _bf_hashset_resize allows new_cap == set->len — guard changed from < to <=
• Document silent -EEXIST dedup in bf_set_add_elem — Doxygen now documents "if it is not already present"
• Clarify bf_hashset_foreach delete-safety — src/libbpfilter/include/bpfilter/core/hashset.h:107 — documentation should note that only the current element is safe to delete during iteration; deleting a different element could corrupt the pre-captured __next pointer

Nits

• Transient dangling state in bf_hashset_clean — src/libbpfilter/core/hashset.c:201 — set->head and slot pointers are stale between foreach frees and final cleanup; add a comment noting intentional ordering
• Test harness accesses hashset internals — tests/harness/test.c:262 — bft_set_eq_ordered reads elems.head directly despite API warning against field access
• bf_set_add_many copies instead of transferring ownership — src/libbpfilter/set.c:498 — doubles per-element allocation vs old bf_list_node_take_data approach; note for future optimization
• Document tombstone sentinel assumption — src/libbpfilter/core/hashset.c:14 — (bf_hashset_elem *)1 relies on address 0x1 never being a valid pointer; add a brief comment

CLAUDE.md improvements

• The __next variable in bf_hashset_foreach (and existing bf_list_foreach/bf_list_foreach_rev) uses a double-underscore prefix reserved by the C standard (ISO C 7.1.3). The PR author has already flagged this as an open thread. Consider a project-wide convention change (e.g., _bf_* prefix) or documenting the intentional use of GCC extensions in CLAUDE.md

Workflow run

[qdeslandes]

Alright, first pass of review and a few things to fix. I'll do a second, deeper pass, when those are solved. That being said, it's a very welcome addition! :D

[pzmarzly]

Ready for review. Remaining open threads:

1. The code here is not proper C17, and uses GCC extensions. Many other places in the codebase do, actually, and pedantic_c test doesn't check macros. Wanna make a follow up? Or give up on pure C17? lib: implement hashset #470 (comment)
2. Would a bf_hashset_take function make sense? What should it return? lib: implement hashset #470 (comment)
3. If someone calls bf_hashset_delete and then tries to use the deleted data, what should happen? Same problem with bf_list_delete, by the way. FWIW I think Claude is overly pedantic. lib: implement hashset #470 (comment)
4. The structure is optimized for foreach speed, not for random access. lib: implement hashset #470 (comment)

[github-actions]

Claude: nit: bf_hashset_foreach doc should clarify dangling pointer after bf_hashset_delete

The doc says "Safe to bf_hashset_delete the current element during iteration" but does not mention that elem_var becomes a dangling pointer after the delete call and must not be accessed in the same iteration body. Consider:

Safe to call `bf_hashset_delete()` on the current element during iteration.
Do not access `elem_var` after the delete call.

[pzmarzly]

I think it's obvious?

[github-actions]

Claude: nit: bf_hashset_elem.data accessor pattern contradicts @warning

The @warning on bf_hashset_elem (line 44) says "the inside of this structure should not be directly accessed", yet the bf_hashset_foreach macro doc (line 107) instructs users to "Access the stored data via elem_var->data". This is the intended access pattern (there's no getter function like bf_list_node_get_data). Either relax the @warning to exempt data, or add a bf_hashset_elem_get_data() inline accessor for consistency with the list API.

[pzmarzly]

Same pattern in bf_list_foreach. Macros need access to struct internals.

[github-actions]

Claude: nit: bft_set_eq_ordered directly accesses bf_hashset struct internals

lhs->elems.head and n0->next bypass the public API, which the @warning on bf_hashset discourages. This could be rewritten using bf_hashset_foreach:

bf_hashset_elem *n1 = rhs->elems.head; // or use a second manual iteration
bf_hashset_foreach (&lhs->elems, n0) {
    if (!n1 || memcmp(n0->data, n1->data, lhs->elem_size) != 0)
        return false;
    n1 = n1->next;
}

Alternatively, since this is test harness code, a comment noting the intentional internal access would suffice.

[pzmarzly]

Test code, should be fine.

[github-actions]

Claude: nit: The old list-based code used bf_list_node_take_data to transfer ownership from to_add to dest without copying. The new implementation calls bf_set_add_elem which does malloc + memcpy for each element. This doubles per-element allocations for large sets. Worth noting as a potential future optimization (e.g., a bf_hashset_take that moves an element between sets without reallocation).

[pzmarzly]

Fair point, something like bf_hashset_take_node and bf_hashset_add_node` (that rewires existing bf_hashset_elem*) would work, though I'm not sure if it's worth it?

In my intuition, a free(<bf_hashset_elem*>) followed by malloc(sizeof(bf_hashset_elem)) should be pretty cheap, since we have a perfect gap of unallocated memory to reuse.