From: | Corey Huinker <corey(dot)huinker(at)gmail(dot)com> |
---|---|
To: | Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> |
Cc: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Daniel Verite <daniel(at)manitou-mail(dot)org>, PostgreSQL <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless) |
Date: | 2017-02-01 13:18:24 |
Message-ID: | CADkLM=fpemjcpyuRKHDLNs51yQANng-Siq=+FOw2FqJfEJFq=Q@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
>
> - 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
From | Date | Subject | |
---|---|---|---|
Next Message | Amit Kapila | 2017-02-01 13:20:29 | Re: Parallel Index Scans |
Previous Message | Corey Huinker | 2017-02-01 13:03:18 | Re: \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless) |