On 09.10.25 09:15, Chao Li wrote:
> 0001 - looks good to me. Basically it only moves for loop’s loop
> variable type definition into for(), it isn’t tied to rest commits, I
> guess it can be pushed independently.
>
> 0002 - also looks good. It just add “const” where possible. I think it
> can be pushed together with 0001.
>
> 0003 - also looks good. It moves “reset_extra” from individual typed
> config structure to “config_generic”, which dramatically eliminates
> unnecessary switch-cases.
I have committed these first three. Attached is the rest of the patch
series rebased.
It looks like we have consensus in principle on the remaining changes,
but I'll leave them out here a while longer in case there are any
further thoughts.
Regarding ...
> Just a small comment:
>
> ```
> @@ -6244,29 +6217,11 @@ RestoreGUCState(void *gucstate)
> switch (gconf->vartype)
> {
> case PGC_BOOL:
> -{
> -struct config_bool *conf = (struct config_bool *) gconf;
> -
> -if (conf->reset_extra && conf->reset_extra != gconf->extra)
> -guc_free(conf->reset_extra);
> -break;
> -}
> case PGC_INT:
> -{
> -struct config_int *conf = (struct config_int *) gconf;
> -
> -if (conf->reset_extra && conf->reset_extra != gconf->extra)
> -guc_free(conf->reset_extra);
> -break;
> -}
> case PGC_REAL:
> -{
> -struct config_real *conf = (struct config_real *) gconf;
> -
> -if (conf->reset_extra && conf->reset_extra != gconf->extra)
> -guc_free(conf->reset_extra);
> -break;
> -}
> +case PGC_ENUM:
> +/* no need to do anything */
> +break;
> case PGC_STRING:
> {
> struct config_string *conf = (struct config_string *) gconf;
> @@ -6274,19 +6229,11 @@ RestoreGUCState(void *gucstate)
> guc_free(*conf->variable);
> if (conf->reset_val && conf->reset_val != *conf->variable)
> guc_free(conf->reset_val);
> -if (conf->reset_extra && conf->reset_extra != gconf->extra)
> -guc_free(conf->reset_extra);
> -break;
> -}
> -case PGC_ENUM:
> -{
> -struct config_enum *conf = (struct config_enum *) gconf;
> -
> -if (conf->reset_extra && conf->reset_extra != gconf->extra)
> -guc_free(conf->reset_extra);
> break;
> }
> }
> ```
>
> Do we still need this switch-case? As only PGC_STRING needs to do
> something, why not just do:
>
> ```
> If (gconf-vartype == PGC_STRING)
> {
> …
> }
> if (gconf->reset_extra && gconf->reset_extra != gconf->extra)
> guc_free(gconf->reset_extra);
> ```
I initially had it like that, but there are a couple of other places in
the existing code that have a switch with only actual code for
PGC_STRING, so I ended up following that pattern.
