-
-
Save rendello/49c23aeba3734b9a3e6c443e9a2e493b to your computer and use it in GitHub Desktop.
| /* | |
| ** 2025-01-01 | |
| ** | |
| ** The author disclaims copyright to this source code. | |
| */ | |
| #include "sqlite3ext.h" | |
| SQLITE_EXTENSION_INIT1 | |
| #include <stdlib.h> | |
| #include <stdio.h> | |
| /* | |
| ** Convert hex string to integer. Allow NULL to pass through, | |
| ** otherise the value must be a non-empty hex string with an optional | |
| ** '0x' or '0X' prefix. | |
| */ | |
| static void hextoint( | |
| sqlite3_context *context, | |
| int argc, | |
| sqlite3_value **argv | |
| ){ | |
| sqlite_int64 result; | |
| int base; | |
| const unsigned char *start; | |
| char *stop; | |
| sqlite3_value *value = argv[0]; | |
| int value_type = sqlite3_value_type(value); | |
| switch (value_type) { | |
| case SQLITE_NULL: | |
| sqlite3_result_null(context); | |
| return; | |
| case SQLITE_TEXT: | |
| start = sqlite3_value_text(value); | |
| result = strtoll((const char *)start, &stop, 16); | |
| if (*start == '\0' || *stop != '\0') | |
| break; | |
| sqlite3_result_int64(context, result); | |
| return; | |
| } | |
| sqlite3_result_error(context, "expects hex string or NULL", -1); | |
| } | |
| #ifdef _WIN32 | |
| __declspec(dllexport) | |
| #endif | |
| int sqlite3_hextoint_init( | |
| sqlite3 *db, | |
| char **pzErrMsg, | |
| const sqlite3_api_routines *pApi | |
| ){ | |
| int rc = SQLITE_OK; | |
| SQLITE_EXTENSION_INIT2(pApi); | |
| (void)pzErrMsg; /* Unused parameter */ | |
| sqlite3_create_function(db, "hextoint", 1, SQLITE_UTF8|SQLITE_DETERMINISTIC, 0, hextoint, 0, 0); | |
| return rc; | |
| } |
I’m following up on your request for a review on LinkedIn. While I don’t work directly with SQL, I do have a lot of experience with C, so I’d be happy to share some thoughts.
First off, welcome to the world of C and the many adventures that come with it! Your function is a great start—clean and simple. With a few small tweaks, it can be even more polished and professional. Here’s what I noticed:
Code review
-
int base;is declared but not used.- If the intent to was use this to control how
strtoll()interprets the string, then go ahead and implement it fully. - I think the cleaner solution is to remove the unused declaration.
- You should be getting a warning for this when you compile your code. I suggest you review your warnings and fix them.
- If the intent to was use this to control how
-
argcis untested, indeed unused.- Testing
argcwill allow you to gracefully exit if the wrong number of arguments are passed in. - If you don't intend to check
argc, you can tell the compiler that it is an unused parameter by using the statement(void) argc;
- You should be getting a warning about this when you compile your code.
- Testing
Pro tips
About Warnings
Too many "benign" warnings in your code make it hard to see critical warnings.
Rather than changing the warning level to hide the noise, you should clean the code to eliminate the warnings.
Many professional teams set up their build flow to treat warnings as errors - the build fails with any warnings.
Magic Numbers
- Avoid using hard-coded "magic" numbers, as they can make the code harder to read and maintain. For example:
- The
16in yourstrtoll()call is the numeric base (radix) for hexadecimal, but this may not be immediately clear to someone reviewing the code.
Consider using named constants for clarity:
#define RADIX_BINARY 2 #define RADIX_OCTAL 8 #define RADIX_DECIMAL 10 #define RADIX_HEX 16
- The
- Similarly, your use of
-1as an error code could benefit from a named constant or an enum. While this isn’t a major issue, adding named error codes can improve readability and make debugging easier. There may be a convention for SQLite that I am unaware of.
@aodonnell-ca Thanks for the review, I was a bit disappointed there was no discussion in the Linked group when I posted it. It's true I missed int base and a few other things, I don't recall being warned, oddly enough. I should check my environment and maybe consider erroring out on warnings.
Was this ChatGPT generated, by the way?
I actually wrote the original draft, markdown and all, but I wondered how my tone was. I handed it into ChatGPT and it noodled around a bit. Hence the language being finessed. I was on a coffee break, so was playing with Chat to help my writing style.
https://chatgpt.com/share/67928f6b-7d1c-8011-a13f-465d85aa57ab - you'll have to search to see the dialog switch to where I discuss this review, as it was in an already open session.
The Pre Chat review was as follows:
I'm following up on your request on LinkedIn for a review.
I don't do SQL, but I do C. There are a few questions I have.
int base;is declared but not used. Was the intent to use this to control howstrtoll()interprets the string? i.e. hex versus octal versus binary versus decimal.- This should cause a compiler warning. It's a good practice to treat warnings as seriously as errors.
argcis untested, indeed unused.- Testing argc will allow you to gracefully exit if the wrong number of arguments are passed in.
- If it's unused, you should be getting a warning. To tell the compiler you know it's unused, you can use the statement
(void) argc;before you consumeargv[].
About Warnings
Too many "benign" warnings in your code make it hard to see critical warnings.
Your unused base will be optimized out by the compiler, so it's not a big concern in itself.
Your ignoring argc might be defensible, considering the consumer of argv can return an appropriate error state.
The more of these warnings you have, the less chance you have of spotting important warnings in your code.
Magic Numbers
Hard-coded "magic" numbers in code are regarded as not best practices. The code becomes more self documenting when named constants are used.
The 16 in stroll(), if you are reviewing the code and not famliar with stroll, you wouldn't necessarily know if that was a string length limit or a radix. Consider using the following defines for readability.
#define RADIX_BINARY 2
#define RADIX_OCTAL 8
#define RADIX_DECIMAL 10
#define RADIX_HEX 16I see you also use a -1 as an error code. I don't know if this is a limitation or convention for SQLLite. If you are permitted other error codes, then again an enum or #define would help out. In this case it's not so great a foul.
@aodonnell-ca I figured, since I looked up your blog and saw you mention you did something similar there. I figured most C programmers wouldn't have an h1 heading entitled "pro tips", that's more of an LLM thing ;) For what it's worth, I think your original tone was good.
I think I'd disagree about the magic numbers in this case, I feel the fact that strtoll takes a radix parameter, coupled with the fact that my short function is called hextoint and has a documentation comment means that adding a #define for a RADIX_HEX constant would detract from the code, in my opinion.
Funnily enough I've also been working with code where strtok seems like the first choice, but eventually I too decided not to use it. It's certainly an old-C-style function what with the in place modification.
Thanks again for taking time out of your lunch break to help review my code, I appreciate it :)
"It looks like it should work. I don't see any obvious errors." -D. Richard Hipp