Verstable icon indicating copy to clipboard operation
Verstable copied to clipboard

Support const char* keys

Open kovidgoyal opened this issue 1 year ago • 3 comments

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 avatar Jul 08 '24 06:07 kovidgoyal

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 \

kovidgoyal avatar Jul 08 '24 06:07 kovidgoyal

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.

JacksonAllan avatar Jul 12 '24 17:07 JacksonAllan

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.

kovidgoyal avatar Jul 13 '24 03:07 kovidgoyal

Hi @JacksonAllan - wondering whether you're going to port https://github.com/JacksonAllan/CC/commit/b67fabebaec2f608d7e970f0569c1d0d7cbaeec7 to Verstable?

duke13137 avatar Mar 05 '25 23:03 duke13137

@fonghou, absolutely! Updating Verstable to include this improvement, probably via @kovidgoyal pull request (apologies for neglecting it until now), is next on the to-do list after the new version of CC is released.

JacksonAllan avatar Mar 06 '25 01:03 JacksonAllan