Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands:\quit_if, \quit_unless)
| От | Corey Huinker |
|---|---|
| Тема | Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands:\quit_if, \quit_unless) |
| Дата | |
| Msg-id | CADkLM=fpemjcpyuRKHDLNs51yQANng-Siq=+FOw2FqJfEJFq=Q@mail.gmail.com обсуждение исходный текст |
| Ответ на | Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands:\quit_if, \quit_unless) (Fabien COELHO <coelho@cri.ensmp.fr>) |
| Ответы |
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)
(Tom Lane <tgl@sss.pgh.pa.us>)
Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands:\quit_if, \quit_unless) (Erik Rijkers <er@xs4all.nl>) Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands:\quit_if, \quit_unless) (Fabien COELHO <fabien.coelho@mines-paristech.fr>) |
| Список | 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
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Corey HuinkerДата:
Сообщение: Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands:\quit_if, \quit_unless)