Re: [PATCH v2 08/12] util: hash: Don't use 'const' with virHashTablePtr
On Thu, Nov 05, 2020 at 10:50:23 -0300, Daniel Henrique Barboza wrote:
On 11/4/20 2:05 PM, Peter Krempa wrote:
> We didn't use it rigorously and some helpers even cast it away. Remove
> const from all hash utility functions.
Unrelated question: how much do we care about standardizing a bit the
'const' usage in the code? I for one had to rely on casting to satisfy
the conditions of "const" variables, that most of time are created because
some functions requires a "const" argument.
I can drop some patches here and there when I found these instances, but
I'm not sure if we want to get rid of 'const', as you're doing here,
or we want to make sure that all function args that aren't going to be
changed must always be 'const'. I prefer slightly the second approach
but I don't mind get away with the 'const' as well.
In this specific instance, this prepares for change to glibs
GHashTable. The glib APIs don't use const so we can't do much.
Additionally, it in some cases really depends on the semantics you want
to achieve (not what actually C allows you):
> diff --git a/src/util/virhash.h b/src/util/virhash.h
> index b00ab0447e..3af4786e9b 100644
> --- a/src/util/virhash.h
> +++ b/src/util/virhash.h
> @@ -60,7 +60,7 @@ typedef int (*virHashSearcher) (const void *payload, const char
*name,
> virHashTablePtr virHashNew(virHashDataFree dataFree);
> virHashAtomicPtr virHashAtomicNew(virHashDataFree dataFree);
> void virHashFree(virHashTablePtr table);
> -ssize_t virHashSize(const virHashTable *table);
> +ssize_t virHashSize(virHashTablePtr table);
For example here we really don't modify the hash table at all, so const
makes sense.
> @@ -88,8 +88,8 @@ void virHashRemoveAll(virHashTablePtr table);
> /*
> * Retrieve the userdata.
> */
> -void *virHashLookup(const virHashTable *table, const char *name);
But virHashLookup returns the pointer to the payload, which you then can
modify. So while you don't modify the hash table container, you are able
to modify the payload. So one can see that as modifying the hash table
itself.
> -bool virHashHasEntry(const virHashTable *table, const char
*name);
> +void *virHashLookup(virHashTablePtr table, const char *name);
> +bool virHashHasEntry(virHashTablePtr table, const char *name);
Here again we don't modify it ...
> @@ -127,8 +127,8 @@ virHashKeyValuePairPtr
virHashGetItems(virHashTablePtr table,
> * of two keys.
> */
> typedef int (*virHashValueComparator)(const void *value1, const void *value2);
> -bool virHashEqual(const virHashTable *table1,
> - const virHashTable *table2,
> +bool virHashEqual(virHashTablePtr table1,
> + virHashTablePtr table2,
> virHashValueComparator compar);
Same here, this is just for the glib compatibility.
> @@ -139,7 +139,7 @@ int virHashForEach(virHashTablePtr table,
virHashIterator iter, void *opaque);
> int virHashForEachSafe(virHashTablePtr table, virHashIterator iter, void
*opaque);
> int virHashForEachSorted(virHashTablePtr table, virHashIterator iter, void
*opaque);
> ssize_t virHashRemoveSet(virHashTablePtr table, virHashSearcher iter, const void
*opaque);
> -void *virHashSearch(const virHashTable *table, virHashSearcher iter,
> +void *virHashSearch(virHashTablePtr table, virHashSearcher iter,
> const void *opaque, char **name);
But in this case you again get the payload pointer which you can modify.
In general C's const keyword is a hint for the programmer at best, in
many cases it doesn't do what you want and specifically in case of
structs it doesn't guarantee that the internals can't be changed, so I'd
not really bother that much with being rigorous in this regards.