Tom Lane wrote:
> "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:
Thanks for looking into this!
> 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.
Agreed, if never clobbering the variable, having the valid/invalid state
returned by ParseVariableBool() allows for simpler code. I'm changing it
that way.
> Also, why aren't you using ParseVariableBool's existing ability to report
> errors?
Its' because there are two cases:
- either only a boolean can be given to the command or variable,
in which we let ParseVariableBool() tell: unrecognized value "bogus" for "command": boolean expected
- or other parameters besides boolean are acceptable, in which case we
can't say "boolean expected", because in fact a boolean is no more or
less expected than the other valid values.
We could shave code by reducing ParseVariableBool()'s error message to: unrecognized value "bogus" for "name"
clearing the distinction between [only booleans are expected]
and [booleans or enum are expected].
Then almost all callers that have their own message could rely
on ParseVariableBool() instead, as they did previously.
Do we care about the "boolean expected" part of the error message?
> 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 get the idea, except that in this example, the compiler is
unhappy because popt->topt.expanded is not a bool, and an
explicit cast here would be kludgy.
For the error printing part, it would go away with the above
suggestion
Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
