Re: [HACKERS] Improvements in psql hooks for variables

"Daniel Verite" <daniel@manitou-mail.org> writes:
> [ psql-var-hooks-v6.patch ]

I took a quick look through this.  It seems to be going in generally
the right direction, but here's a couple of thoughts:

* It seems like you're making life hard for yourself, and confusing for
readers, by having opposite API conventions at different levels.  You've
got ParseVariableBool returning the parsed value as function result with
validity flag going into an output parameter, but the boolean variable
hooks do it the other way.

I'm inclined to think that the best choice is for the function result
to be the ok/not ok flag, and pass the variable-to-be-modified as an
output parameter.  That fits better with the notion that the variable
is not to be modified on failure.  You're having to change every caller
of ParseVariableBool anyway, so changing them a bit more doesn't seem
like a problem.  I think actually you don't need generic_boolean_hook()
at all if you do that; it appears to do nothing except fix this impedance
mismatch.

Also, why aren't you using ParseVariableBool's existing ability to report
errors?  It seems like this:
         else if (value)
!         {
!             bool    valid;
!             bool    newval = ParseVariableBool(value, NULL, &valid);
!             if (valid)
!                 popt->topt.expanded = newval;
!             else
!             {
!                 psql_error("unrecognized value \"%s\" for \"%s\"\n",
!                            value, param);
!                 return false;
!             }
!         }

is really the hard way and you could have just done

-             popt->topt.expanded = ParseVariableBool(value, param);
+             success = ParseVariableBool(value, param, &popt->topt.expanded);


I'd do it the same way for ParseCheckVariableNum.  Also, is there a reason
why that's adding new code rather than changing ParseVariableNum?
I think if we're going to have a policy that bool variables must be valid
bools, there's no reason not to insist similarly for integers.

* More attention is needed to comments.  Most glaringly, you've changed
the API for VariableAssignHook without any attention to the API spec
above that typedef.  (That comment block is a bit confused anyway, since
half of it is an overall explanation of what the module does and the
other half is an API spec for the hooks.  I think I'd move the overall
explanation into the file header comment.)  Also, I don't like this way
of explaining an output parameter:

+  * "valid" points to a bool reporting whether the value was valid.

because it's not really clear that the function is setting that value
rather than expecting it to be set to something by the caller.
Assuming you take my advice in the previous point, you could document
ParseVariableBool and ParseVariableNum along the lines of
* Returns true if string contents represent a valid value, false if not.* If the string is valid, the value is stored
into*value, else *value* remains unchanged.
 

        regards, tom lane