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.
On a detailed look over this, you are right Laurenz about this.
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.
Hmm. actually I don't have any good answers to this locale issue.
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.
