Hello Corey,

Some comments about v4:

Patch applies. "git apply" complained about a space or line somewhere, not sure why. make check ok.

- rebased on post-psql hooks master


- took nearly every suggestion for change to documentation

Indeed. Looks ok to me.

- \if ERROR will throw the entire \if..\endif into IGNORE mode

Ok. I think that it is the better behavior, but other people opinion may differ. Opinions are welcome.

- state is now pushed on all \ifs


- reinstated leveraging of ParseVariableBool


- history is now kept in interactive mode regardless of \if-truth


- reworked coding example to cause less agita


- removed MainLoop "are endifs balanced" variable in favor of in-place
check which respects ON_ERROR_STOP.


- make changes to psql/Makefile to enable TAP tests and created t/ directory
- 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.

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...).

I'm hoping my failure in that last bit is easy to spot/fix, so I can move
forward with testing unbalanced branching, etc.

Other comments and suggestions:

Variable "slashCmdStatus" is set twice in 3 lines in "mainloop.c"?

There is a spurious empty line added at the very end of "mainloop.h":

   #endif   /* MAINLOOP_H */

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"...

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.

"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"...

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?

  +       return ((s == IFSTATE_NONE) ||
  +                       (s == IFSTATE_TRUE) ||
  +                       (s == IFSTATE_ELSE_TRUE));

I would remove all parenthenses.

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



Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to