Re: \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

>
> - wrote an intentionally failing TAP test to see if "make check" executes
>> it. it does not. need help.
>>
>
> A failing test expects code 3, not 0 as written in "t/001_if.pl". With
>
> is($retcode,'3','Invalid \if respects ON_ERROR_STOP');
>
> instead, the tests succeeds because psql returned 3.
>

Right. What I meant was, no matter how I changed that test, I could not get
it to fail, which made me think it was not executing at all. What do I need
to do to get TAP tests running? I must be missing something.

> More check should be done about stdout to check that it failed for the
> expected reasons though. And maybe more tests could be added to trigger all
> possible state transition errors (eg else after else, elif after else,
> endif without if, if without endif, whatever...).

Agreed. But I couldn't build further on the test without being sure it was
being run.

>
> Other comments and suggestions:
>
> Variable "slashCmdStatus" is set twice in 3 lines in "mainloop.c"?
>

I think that's a merge error from rebasing. Will fix.

>
> There is a spurious empty line added at the very end of "mainloop.h":
>
> +
> #endif /* MAINLOOP_H */
>

Not in my diff, but that's been coming and going in your diff reviews.

>
>
> I would suggest to add a short one line comment before each test to
> explain what is being tested, like "-- test \elif execution", "-- test
> \else execution"...
>

Where are you suggesting this?

>
> Debatable suggestion about "psql_branch_empty": the function name somehow
> suggests an "empty branch", where it is really testing that the stack is
> empty. Maybe the function could be removed and "psql_branch_get_state(...)
> == IF_STATE_NONE" used instead. Not sure.
>

The name isn't great. Maybe psql_branch_stack_empty()?

"psql_branch_end_state": it is a pop, it could be named "psql_branch_pop"
> which would be symmetrical to "psql_branch_push"? Or maybe push should be
> named "begin_state" or "new_state"...
>

Yeah, we either need to go fully with telling the programmer that it's a
stack (push/pop/empty) or (begin_branch/end_branch/not_branching). I'm
inclined to go full-stack, as it were.

>
> C style details: I would avoid non mandatory parentheses, eg:
>
> + return ((strcmp(cmd, "if") == 0 || \
> + strcmp(cmd, "elif") == 0 || \
> + strcmp(cmd, "else") == 0 || \
> + strcmp(cmd, "endif") == 0));
>
> I would remove the external double parentheses (( ... )). Also I'm not
> sure why there are end-of-line backslashes on the first instance, maybe a
> macro turned into a function?
>

I copied that from somewhere, and I don't remember where. I think the test
was originally nested much further before being moved to its own function.
Can fix.

>
> + return ((s == IFSTATE_NONE) ||
> + (s == IFSTATE_TRUE) ||
> + (s == IFSTATE_ELSE_TRUE));
>
> I would remove all parenthenses.
>

+1

>
> + return (state->branch_stack == NULL);
>
> Idem.

+1