Hello Corey,
> v24 highlights:
>
> - finally using git format-patch
> - all conditional slash commands broken out into their own functions
> (exec_command_$NAME) , each one tests if it's in an active branch, and if
> it's not it consumes the same number of parameters, but discards them.
> comments for each slash-command family were left as-is, may need to be
> bulked up.
> - TAP tests discarded in favor of one ON_EROR_STOP test for invalid \elif
> placement
> - documentation recommending ON_ERROR_STOP removed, because invalid
> expressions no longer throw off if-endif balance
> - documentation updated to reflex that contextually-correct-but-invalid
> boolean expressions are treated as false
> - psql_get_variable has a passthrough void pointer now, but I ended up not
> needing it. Instead, all slash commands in false blocks either fetch
> OT_NO_EVAL or OT_WHOLE_LINE options. If I'm missing something, let me know.
A few comments about the patch.
Patch applies. "make check" ok.
As already pointed out, there is one useless file in the patch.
Although currently there is only one expected argument for boolean
expressions, the n² concatenation algorithm in gather_boolean_expression
is not very elegant. Is there some string buffer data structure which
could be used instead?
ISTM that ignore_boolean_expression may call free on a NULL pointer if the
expression is empty?
Generally I find the per-command functions rather an improvement.
However there is an impact on testing because of all these changes. ISTM
that test cases should reflect this situation and test that \cd, \edit,
... are indeed ignored properly and taking account there expected args...
In "exec_command_connect" an argument is changed from "-reuse-previous" to
"-reuse-previous=", not sure why.
There seems to be pattern repetition for _ev _ef and _sf _sv. Would it
make sense to create a function instead of keeping the initial copy-paste?
I think that some functions could be used for some repeated cases such as
"discard one arg", "discard one or two arg", "discard whole line", for the
various inactive branches, so as to factor out code.
I would suggest to put together all if-related backslash command,
so that the stack management is all in one function instead of 4.
For pset the inactive branch does OT_NORMAL instead of OT_NOT_EVAL, not
sure why.
--
Fabien.
