Re: [PATCH v2 08/12] util: hash: Don't use 'const' with virHashTablePtr
On 11/5/20 12:26 PM, Peter Krempa wrote: 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. [...] 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. It's also easier to deal without 'const' here and there in the code. I mean, as you said, it's more of a hint to the developer. One that comes with the cost of adding casts here and there when needed. Thanks for the reply. Guess I'll keep an eye for the 'const' cases I come across from now on and see if it can be removed/simplified. DHB
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.
Re: [PATCH v2 08/12] util: hash: Don't use 'const' with virHashTablePtr
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.
Thanks,
DHB
Signed-off-by: Peter Krempa
---
src/conf/nwfilter_params.c | 2 +-
src/conf/nwfilter_params.h | 2 +-
src/util/virhash.c | 17 -
src/util/virhash.h | 12 ++--
4 files changed, 16 insertions(+), 17 deletions(-)
diff --git a/src/conf/nwfilter_params.c b/src/conf/nwfilter_params.c
index dd2b92c97b..33bd7437f1 100644
--- a/src/conf/nwfilter_params.c
+++ b/src/conf/nwfilter_params.c
@@ -983,7 +983,7 @@ virNWFilterVarAccessGetIntIterId(const virNWFilterVarAccess
*vap)
bool
virNWFilterVarAccessIsAvailable(const virNWFilterVarAccess *varAccess,
-const virHashTable *hash)
+virHashTablePtr hash)
{
const char *varName = virNWFilterVarAccessGetVarName(varAccess);
const char *res;
diff --git a/src/conf/nwfilter_params.h b/src/conf/nwfilter_params.h
index 4962a86864..b91f830c31 100644
--- a/src/conf/nwfilter_params.h
+++ b/src/conf/nwfilter_params.h
@@ -119,7 +119,7 @@ virNWFilterVarAccessType virNWFilterVarAccessGetType(
unsigned int virNWFilterVarAccessGetIterId(const virNWFilterVarAccess *vap);
unsigned int virNWFilterVarAccessGetIndex(const virNWFilterVarAccess *vap);
bool virNWFilterVarAccessIsAvailable(const virNWFilterVarAccess *vap,
- const virHashTable *hash);
+ virHashTablePtr hash);
typedef struct _virNWFilterVarCombIterEntry virNWFilterVarCombIterEntry;
typedef virNWFilterVarCombIterEntry *virNWFilterVarCombIterEntryPtr;
diff --git a/src/util/virhash.c b/src/util/virhash.c
index 4f8df8fae3..a5ab48dbf5 100644
--- a/src/util/virhash.c
+++ b/src/util/virhash.c
@@ -360,7 +360,8 @@ virHashGetEntry(const virHashTable *table,
* Returns a pointer to the userdata
*/
void *
-virHashLookup(const virHashTable *table, const char *name)
+virHashLookup(virHashTablePtr table,
+ const char *name)
{
virHashEntryPtr entry = virHashGetEntry(table, name);
@@ -381,7 +382,7 @@ virHashLookup(const virHashTable *table, const char *name)
* Returns true if the entry exists and false otherwise
*/
bool
-virHashHasEntry(const virHashTable *table,
+virHashHasEntry(virHashTablePtr table,
const char *name)
{
return !!virHashGetEntry(table, name);
@@ -434,7 +435,7 @@ virHashAtomicSteal(virHashAtomicPtr table,
* -1 in case of error
*/
ssize_t
-virHashSize(const virHashTable *table)
+virHashSize(virHashTablePtr table)
{
if (table == NULL)
return -1;
@@ -652,15 +653,13 @@ virHashRemoveAll(virHashTablePtr table)
* The elements are processed in a undefined order. Caller is
* responsible for freeing the @name.
*/
-void *virHashSearch(const virHashTable *ctable,
+void *virHashSearch(virHashTablePtr table,
virHashSearcher iter,
const void *opaque,
char **name)
{
size_t i;
-/* Cast away const for internal detection of misuse. */
-virHashTablePtr table = (virHashTablePtr)ctable;
if (table == NULL || iter == NULL)
return NULL;
@@ -739,7 +738,7 @@ virHashGetItems(virHashTablePtr table,
struct virHashEqualData
{
bool equal;
-const virHashTable *table2;
+virHashTablePtr table2;
virHashValueComparator compar;
};
@@ -760,8 +759,8 @@ static int virHashEqualSearcher(const void *payload, const
char *name,
return 0;
}
-bool virHashEqual(const virHashTable *table1,
- const virHashTable *table2,
+bool virHashEqual(virHashTablePtr table1,
+ virHashTablePtr table2,
virHashValueComparator compar)
{
struct virHashEqualData data = {
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
