On Thu, Mar 2, 2017 at 1:23 AM, Fabien COELHO <coe...@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 boolean
is_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)) ?
