[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: A new module: bitset
From: |
Bruno Haible |
Subject: |
Re: A new module: bitset |
Date: |
Sun, 25 Nov 2018 16:44:33 +0100 |
User-agent: |
KMail/5.1.3 (Linux/4.4.0-138-generic; KDE/5.18.0; x86_64; ; ) |
Hi Akim,
This code looks like well-designed, high-quality and very suitable for
gnulib.
I love especially about it that it's adaptable as programs change:
Initially the program might require fixed-size bitsets, and later
move to variable-size bitsets.
> I think it should move to gnulib
Yes, definitely.
> I have written a short
> doc so that the reader can get at least a sense of how to use this
> library.
This doc is good and essential (to get an overview).
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'.
* 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?
* 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?
* 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.
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.)
* The hint BITSET_FRUGAL is unused. How about eliminating it?
Bruno
Re: A new module: bitset, Akim Demaille, 2018/11/25
Re: A new module: bitset, Paul Eggert, 2018/11/25