Thanks for the review!
On Wed, 2024-07-24 at 15:27 +0200, Rafia Sabih wrote:
> I liked the idea for this patch. I will also go for the default being
> an empty string.
> I went through this patch and have some comments on the code,
>
> 1. In general, I don't like the idea of goto, maybe we can have a
> free_something function to call here.
The PostgreSQL code base has over 3000 goto's...
Sure, that can be factored out to a function (except the final "return"),
but I feel that a function for three "free" calls is code bloat.
Do you think that avoiding the goto and using a function would make the
code simpler and clearer?
> 2.
> if (!SplitIdentifierString(new_copy, ',', &states))
> {
> GUC_check_errdetail("List syntax is invalid.");
> goto failed;
> }
> Here, we don't need all that free-ing, we can just return false here.
I am OK with changing that; I had thought it was more clearer and more
foolproof to use the same pattern everywhere.
> 3.
> /*
> * Check the the values are alphanumeric and convert them to upper case
> * (SplitIdentifierString converted them to lower case).
> */
> for (p = state; *p != '\0'; p++)
> if (*p >= 'a' && *p <= 'z')
> *p += 'A' - 'a';
> else if (*p < '0' || *p > '9')
> {
> GUC_check_errdetail("error codes can only contain digits and ASCII letters.");
> goto failed;
> }
> I was thinking, maybe we can use tolower() function here.
That is a good idea, but these C library respect the current locale.
I would have to explicitly specify the C locale or switch the locale
temporarily.
Switching the locale seems clumsy, and I have no idea what I would have
to feed as second argument to toupper_l() or isalnum_l().
Do you have an idea?
> 4.
> list_free(states);
> pfree(new_copy);
>
> *extra = statelist;
> return true;
>
> failed:
> list_free(states);
> pfree(new_copy);
> guc_free(statelist);
> return false;
>
> This looks like duplication that can be easily avoided.
> You may have free_somthing function to do all free-ing stuff only and
> its callee can then have a return statement.
> e.g for here,
> free_states(states, new_copy, statelist);
> return true;
That free_states() function would just contain two function calls.
I think that defining a special function for that is somewhat out of
proportion.
> 5. Also, for alphanumeric check, maybe we can have something like,
> if (isalnum(*state) == 0)
> {
> GUC_check_errdetail("error codes can only contain digits and ASCII letters.");
> goto failed;
> }
> and we can do this in the beginning after the len check.
isalnum() operates on a single character and depends on the current locale.
See my comments to 3. above.
Please let me know what you think, particularly about the locale problem.
Yours,
Laurenz Albe