lib: add bf_vector and use it in cgen#487
Conversation
Claude review of PR #487 (0926237)Suggestions
Nits
CLAUDE.md improvements
|
2bbe579 to
419e434
Compare
419e434 to
e77f462
Compare
e77f462 to
305dd19
Compare
305dd19 to
f5a94cd
Compare
f5a94cd to
e9d06a6
Compare
e9d06a6 to
4cd032f
Compare
703e225 to
a8759b5
Compare
a8759b5 to
c828488
Compare
| (elem) && (elem) < (vec)->data + ((vec)->size * (vec)->elem_size); \ | ||
| (elem) = (elem) + (vec)->elem_size) | ||
|
|
||
| /** |
There was a problem hiding this comment.
Claude: suggestion: bf_vector_foreach performs pointer arithmetic on void * ((elem) + (vec)->elem_size and (vec)->data + ...), which is a GCC extension not valid in standard C.
Since vector.h is a public header under include/bpfilter/core/, any downstream consumer using this macro with -pedantic -std=c17 will get a compile error. The pedantic integration test (tests/integration/pedantic_c.sh) includes all public headers with -pedantic-errors -std=c17 -Werror — the macro definition alone won't trigger an error (unexpanded macros aren't compiled), but any call site under those flags will fail.
For comparison, bf_list_foreach in list.h avoids this by iterating over typed pointers (bf_list_node *). Consider casting through char *:
| /** | |
| #define bf_vector_foreach(vec, elem) \ | |
| for (void *(elem) = (vec)->data; \ | |
| (elem) && (char *)(elem) < (char *)(vec)->data + ((vec)->size * (vec)->elem_size); \ | |
| (elem) = (void *)((char *)(elem) + (vec)->elem_size)) |
There was a problem hiding this comment.
We could do that, but the codebase is already not C17 compliant in different places as well.
| * @brief Iterate over every element of a `bf_vector`. | ||
| * | ||
| * `elem` is declared as a pointer to the element type and will point to each | ||
| * element in turn. Do not add or remove elements during iteration. |
There was a problem hiding this comment.
Do not add
This should not be an issue.
remove elements during iteration
Same here, as we don't shrink the memory buffer.
There was a problem hiding this comment.
Do not add
If that addition causes growth/reallocation, void *(elem) = (vec)->data; will point to the old memory address. I could turn the iterator into index int i = 0; and use double-for trick to have void *(elem) = (vec)->data + i * (vec)->elem_size;, but I think it's an overkill?
remove elements during iteration
But we shift the next elements left, i.e. 5th element becomes 4th, 4th becomes 3rd, etc. The foreach will skip an element because of (elem) = (elem) + (vec)->elem_size.
eb242f7 to
9c2bbd0
Compare
d5af70d to
73c0440
Compare
73c0440 to
8ff783d
Compare
| _program->fixups = bf_list_default(bf_fixup_free, NULL); | ||
| _program->handle = handle; | ||
|
|
||
| r = bf_vector_reserve(&_program->img, 512); |
There was a problem hiding this comment.
Claude: suggestion: The initial reservation of 512 instructions (512 * sizeof(struct bpf_insn) = 4 KiB) is 8x larger than the old _BF_PROGRAM_DEFAULT_IMG_SIZE of 64. This may be an intentional tuning improvement to reduce early reallocations, but the rationale is not documented. Consider defining a named constant (like the removed _BF_PROGRAM_DEFAULT_IMG_SIZE) and adding a brief comment explaining the sizing choice.
| /** | ||
| * @brief Get a pointer to the n-th element. | ||
| * | ||
| * @param vec Initialised vector. Must be non-NULL. |
There was a problem hiding this comment.
Claude: nit: The bf_vector_clean documentation says the vector can be "reused (e.g. by re-assigning via bf_vector_default)" but does not mention that elem_size is preserved. Since bf_vector_add works correctly on a cleaned vector without re-initialization, consider noting that elem_size is retained, so callers know whether they need to call bf_vector_default/bf_vector_init again.
There was a problem hiding this comment.
They should call init / default nonetheless. We can be permissive in case they forgot, but then we shouldn't document it.
8ff783d to
0926237
Compare
I initially planned to use bf_vector in bf_hashset, but I got convinced by @yaakov-stein that it's not a good idea. However, we could use bf_vector in other places, especially where we use bf_dynbuf or where we handroll vector-like behavior.
Commits:
lib: core: add bf_vector- implementation. Vector doubles in size when it hits the limits. Has a helper to remove elements as well, though shrinking is not implemented.cgen: use bf_vector for img- Instead of managingimg,img_size, andimg_capby hand, use bf_vector.