JacksonAllan / Verstable

A versatile, performance-oriented generic hash table library for C.
MIT License
69 stars 8 forks source link

Support const char* keys #12

Open kovidgoyal opened 1 week ago

kovidgoyal commented 1 week ago

Currently, vt_hash_string and vt_cmpr_string do not declare their arguments to be const. This makes them not useable with KEY_TY of const char*

Ideally, HASH_FN and CMPR_FN should be set automatically when KEY_TY is const char just like they are for key type char, but that is not so important.

kovidgoyal commented 1 week ago

Here is a patch:

--- /t/verstable.h  2024-07-08 11:44:53.464754276 +0530
+++ 3rdparty/verstable.h    2024-07-08 11:45:43.431669510 +0530
@@ -550,7 +550,7 @@ static inline uint64_t vt_hash_integer(
 }

 // FNV-1a.
-static inline uint64_t vt_hash_string( char *key )
+static inline uint64_t vt_hash_string( const char *key )
 {
   uint64_t hash = 0xcbf29ce484222325ull;
   while( *key )
@@ -564,7 +564,7 @@ static inline bool vt_cmpr_integer( uint
   return key_1 == key_2;
 }

-static inline bool vt_cmpr_string( char *key_1, char *key_2 )
+static inline bool vt_cmpr_string( const char *key_1, const char *key_2 )
 {
   return strcmp( key_1, key_2 ) == 0;
 }
@@ -877,10 +877,10 @@ VT_CAT( NAME, _itr ) VT_CAT( NAME, _eras
 #define HASH_FN                                                               \
 _Pragma( "warning( push )" )                                                  \
 _Pragma( "warning( disable: 4189 )" )                                         \
-_Generic( ( KEY_TY ){ 0 }, char *: vt_hash_string, default: vt_hash_integer ) \
+_Generic( ( KEY_TY ){ 0 }, char *: vt_hash_string, const char*: vt_hash_string, default: vt_hash_integer ) \
 _Pragma( "warning( pop )" )
 #else
-#define HASH_FN _Generic( ( KEY_TY ){ 0 }, char *: vt_hash_string, default: vt_hash_integer )
+#define HASH_FN _Generic( ( KEY_TY ){ 0 }, char *: vt_hash_string, const char*: vt_hash_string, default: vt_hash_integer )
 #endif
 #else
 #error Hash function inference is only available in C11 and later. In C99, you need to define HASH_FN manually to \
@@ -894,10 +894,10 @@ vt_hash_integer, vt_hash_string, or your
 #define CMPR_FN                                                               \
 _Pragma( "warning( push )" )                                                  \
 _Pragma( "warning( disable: 4189 )" )                                         \
-_Generic( ( KEY_TY ){ 0 }, char *: vt_cmpr_string, default: vt_cmpr_integer ) \
+_Generic( ( KEY_TY ){ 0 }, char *: vt_cmpr_string, const char*: vt_cmpr_string, default: vt_cmpr_integer ) \
 _Pragma( "warning( pop )" )
 #else
-#define CMPR_FN _Generic( ( KEY_TY ){ 0 }, char *: vt_cmpr_string, default: vt_cmpr_integer )
+#define CMPR_FN _Generic( ( KEY_TY ){ 0 }, char *: vt_cmpr_string, const char*: vt_cmpr_string, default: vt_cmpr_integer )
 #endif
 #else
 #error Comparison function inference is only available in C11 and later. In C99, you need to define CMPR_FN manually \
JacksonAllan commented 1 week ago

Thanks for working on this! const-correctness throughout the library is planned for the next release (I previously mentioned the matter in #11). This ought to include all function parameters that can be const, including the key parameters of _insert, _get, and _get_or_insert. I'm not sure yet how well this change will mesh with your suggestions, which are intended to allow the user to specify const as part of the key type (i.e. via KEY_TY). For example, there would be duplicate const specifiers on the aforementioned parameters, which would result in compiler warnings. It's possibly that the changes I intend to make will eliminate a user's need to specify a const key type in the first place. I'll need to investigate further.

kovidgoyal commented 1 week ago

These changes dont affect that, since they aren't marking key parameters const. They would be required anyway if you wanted to mark key parameters const, so I see no harm in merging them now.

Note that in my code using verstable.h I extensively use const char* as KEY_TY. Changing verstable to add const to KEY_TY internally would break that. It's not a big deal as I can obviously change my code, but it is a breaking change.