On 04/19/2011 07:22 AM, Jiri Denemark wrote:
> This adds several tests for remaining hash APIs (custom
> hasher/comparator functions are not covered yet, though).
>
> All tests pass both before and after the "Simplify hash implementation".
> ---
> src/util/hash.c | 18 +++
> src/util/hash.h | 1 +
> tests/hashdata.h | 237 +++++++++++++++++++++++++++++++++++-
> tests/hashtest.c | 361 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 615 insertions(+), 2 deletions(-)
>
> diff --git a/src/util/hash.c b/src/util/hash.c
> index fc7652d..5dbc7f1 100644
> --- a/src/util/hash.c
> +++ b/src/util/hash.c
> @@ -499,6 +499,24 @@ virHashSize(virHashTablePtr table)
> }
>
> /**
> + * virHashTableSize:
> + * @table: the hash table
> + *
> + * Query the size of the hash @table, i.e., number of keys in the table.
This is confusing. I think virHashSize is already the number of keys
(or elements) in the table; whereas virHashTableSize is the number of
buckets (> keys if table is not full, < keys if table is using lots of
collision chains but hasn't had a reason to grow). I'd rather use the
term "buckets" than "keys" for this documentation.
> + *
> + * Returns the number of keys in the hash table or
> + * -1 in case of error
> + */
> +int
> +virHashTableSize(virHashTablePtr table)
> +{
> + if (table == NULL)
> + return -1;
> + return table->size;
> +}
Nice, but does it also need an addition to src/libvirt_private.syms?
> "d1a564b2-c7f3-4b76-8712-3b8f5aae6ded",
> - "0e614f33-c1da-4cfe-b6d5-65ecd2d066f2"
> + "0e614f33-c1da-4cfe-b6d5-65ecd2d066f2",
> +/* [250] */ "334fdaba-f373-42ff-8546-219c1463a7c5",
> + "d4ff408e-8d43-46ff-94a4-bcfa6c994776",
> +/* [253] */ "d1abd887-d5de-46b0-88d6-f71f31a61305",
> +/* [254] */ "1d76af65-64d6-4211-b1b5-f5b799595e4d",
> +/* [255] */ "b3ad4257-29b0-4c44-b7a7-95f1d102769c"
Put a trailing comma on this one (if you had done that last time, we
wouldn't have the - vs. + line for 0e614f33... a few lines earlier).
> +const char *uuids_new[] = {
> + "a103cc42-d0e5-40fb-8f7f-3c1bee93e327",
> + "01e13dc5-e65b-4988-a0cc-0d2f1f1e10fe",
> + "71f3678a-a8c6-4176-a26e-c34779067135",
> + "4f054508-22d5-49e1-9962-7508225c8b16",
> + "e572116b-5b7b-45fd-bbe9-296029ce16b5",
> + "695c8cfe-9830-4d9a-be67-50a124cefb76",
> + "ea244996-645b-4a19-ad4a-48f3022b8e94",
> + "0fd98758-9cc4-4779-b403-95ae3500f138",
> + "b86772b4-0728-46ae-99e8-027799697838",
> + "1c0cd559-81cd-4c27-8e24-6aef6f5af7f1",
> + "2a37fe4a-8825-4fd6-9284-e1606967548a",
> + "5920cc9d-62a3-4772-9e73-eb97f0bc483c",
> + "53c215dd-bdba-4fdc-887a-86ab6f860df4"
Trailing comma here, too.
> +static int
> +testHashRemoveSetIter(const void *payload ATTRIBUTE_UNUSED,
> + const void *name,
> + const void *data)
> +{
> + int *count = (int *) data;
> + bool rem = false;
> + int i;
> +
> + for (i = 0; i < ARRAY_CARDINALITY(uuids_subset); i++) {
> + if (STREQ(uuids_subset[i], name)) {
> + rem = true;
> + break;
> + }
> + }
> +
> + if (rem || rand() % 2) {
Do we need to seed the random number pseudo-generator, so that the test
is reproducible? I guess I'm okay if it's not seeded, though.
Overall, this looks like a good patch; the fact that it passes 'make
check' speaks highly, and it is some good additional coverage.
ACK with nits fixed.
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list