Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support const char* keys #12

Open
kovidgoyal opened this issue Jul 8, 2024 · 3 comments · May be fixed by #13
Open

Support const char* keys #12

kovidgoyal opened this issue Jul 8, 2024 · 3 comments · May be fixed by #13

Comments

@kovidgoyal
Copy link

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
Copy link
Author

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 added a commit to kovidgoyal/Verstable that referenced this issue Jul 8, 2024
The builtin hash and comparison functions now mark their arguments as
const, which they should have been doing anyway.

In addition, automatic deduction of hash and cmpr functions now works
for const char* keys.

Fixes JacksonAllan#12
@kovidgoyal kovidgoyal linked a pull request Jul 8, 2024 that will close this issue
@JacksonAllan
Copy link
Owner

JacksonAllan commented Jul 12, 2024

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants