Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands:\quit_if, \quit_unless) - Mailing list pgsql-hackers
| From | Corey Huinker |
|---|---|
| Subject | Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands:\quit_if, \quit_unless) |
| Date | |
| Msg-id | CADkLM=e9BY_-PT96mcs4qqiLtt8t-Fp=e_AdycW-aS0OQvbC9Q@mail.gmail.com Whole thread Raw |
| In response to | Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands:\quit_if, \quit_unless) (Fabien COELHO <coelho@cri.ensmp.fr>) |
| Responses |
Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands:\quit_if, \quit_unless)
(Fabien COELHO <coelho@cri.ensmp.fr>)
|
| List | pgsql-hackers |
On Thu, Mar 2, 2017 at 1:23 AM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
Hello Corey,That is accurate. The only positive it has is that the user only
experiences one error, and it's the first error that was encountered if
reading top-to-bottom, left to right. It is an issue of which we prioritize
- user experience or simpler code.
Hmmm. The last simpler structure I suggested, which is basically the one used in your code before the update, does check for the structure error first. The only drawback is that the condition is only evaluated when needed, which is an issue we can cope with, IMO.
Tom was pretty adamant that invalid commands are not executed. So in a case like this, with ON_ERROR_STOP off:
\if false
\echo 'a'
\elif true
\echo 'b'
\elif invalid
\echo 'c'
\endif
Both 'b' and 'c' should print, because "\elif invalid" should not execute. The code I had before was simpler, but it missed that.
Now if you want to require committer opinion on this one, fine with me.
Rather than speculate on what a committer thinks of this edge case (and
making a patch for each possible theory), I'd rather just ask them what
their priorities are and which user experience they favor.
ISTM that the consistent message by Robert & Tom was to provide simpler code even if the user experience is somehow degraded, as they required that user-friendly features were removed (eg trying to be nicer about structural syntax errors, barking in interactive mode so that the user always knows the current status, providing a detailed status indicator in the prompt...).
Ok, so here's one idea I tossed around, maybe this will strike the right balance for you.
If I create a function like this:
static booleanis_valid_else_context(IfState if_state, const char *cmd){/* check for invalid \else / \elif contexts */
switch (if_state)
{case IFSTATE_NONE:/* not in an \if block */psql_error("\\%s: no matching \\if\n", cmd);return false;break;case IFSTATE_ELSE_TRUE:case IFSTATE_ELSE_FALSE:psql_error("\\%s: cannot occur after \\else\n", cmd);return false;break;default:break;}return true;}
Then the elif block looks something like this:
else if (strcmp(cmd, "elif") == 0)
{
ifState if_state = conditional_stack_peek(cstack);
if (is_valid_else_context(if_state, "elif"))
{
/*
* valid \elif context, check for valid expression
*/
bool elif_true = false;
success = read_boolean_expression(scan_state, "\\elif <expr>",
&elif_true);
if (success)
{
/*
* got a valid boolean, what to do with it depends on current
* state
*/
switch (if_state)
{
case IFSTATE_IGNORED:
/*
* inactive branch, do nothing.
* either if-endif already had a true block,
* or whole parent block is false.
*/
break;
case IFSTATE_TRUE:
/*
* just finished true section of this if-endif,
* must ignore the rest until \endif
*/
conditional_stack_poke(cstack, IFSTATE_IGNORED);
break;
case IFSTATE_FALSE:
/*
* have not yet found a true block in this if-endif,
* this might be the first.
*/
if (elif_true)
conditional_stack_poke(cstack, IFSTATE_TRUE);
break;
default:
/* error cases all previously ruled out */
break;
}
}
}
else
success = false;
psql_scan_reset(scan_state);
}
This is functionally the same as my latest patch, but the ugliness of switching twice on if_state is hidden.
As an added benefit, the "else"-handling code gets pretty simple because it can leverage that same function.
Does that handle your objections?
p.s. do we try to avoid constructs like if (success = my_function(var1, var2)) ?
pgsql-hackers by date: