Hi Bruno! > Le 25 nov. 2018 à 16:44, Bruno Haible <br...@clisp.org> a écrit : > > Some remarks: > > * I can imagine programs that want bitsets but no bitset-vectors. > Suggestion: Move the bitsetv* code to a second module, named > 'bitset-vector'.
That sounds good, indeed. > * File names are nowadays not limited to 8+3 characters, and names > like 'abitset.h', 'bbitset.h', 'ebitset.h', 'lbitset.h', 'vbitset.h' > are not really telling. How about using more descriptive names? > Note: You can use subdirectories to keep files grouped together, > especially since the only file the user sees is bitset.h. > How about renaming e.g. > abitset.[hc] -> bitset/array.[hc] > lbitset.[hc] -> bitset/list.[hc] > and keeping only bitset.h at the top level? Fine with me! > * In the tests, the function > void check_attributes (enum bitset_attr attr) > ignores its attr argument. Probably you meant to use attr instead of > BITSET_FIXED? Good catch, thanks. I wonder why the compiler did not complain. > * Regarding tests: There is a large amount of algorithmic code. While > the parts that Bison uses are surely correct, there is a possibility > of bugs in the more rarely used parts. I would find it very useful > to have unit tests of all 32 operations. I definitely agree. But I'd like to be incremental on this regard. > An approach that minimizes the code to be written would be to > do the same operations on, say, list-bitsets and array-bitsets, and > compare the results. This way, you can infer the correctness of the > list-bitsets implementation, assuming the correctness of the array-bitsets. > (This approach is already used in the test-*list.c tests.) Sure, but that won't catch errors on dispatch that would accidentally map two operations to the same one. So, eventually it would be better to set the real expected results. > * The hint BITSET_FRUGAL is unused. How about eliminating it? My reading of the code was rather that it is there, but needs not be dealt with by the code, since that's the final default case that implements it. So it makes sense from the interface point of view, but needs not be explicitly handled by the code.
