> > > 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? >
I wished for the same thing, happy to use one if it is made known to me. I pulled that pattern from somewhere else in the code, and given that the max number of args for a command is around 4, I'm not too worried about scaling. > > ISTM that ignore_boolean_expression may call free on a NULL pointer if the > expression is empty? > True. The psql code is actually littered with a lot of un-checked free(p) calls, so I started to wonder if maybe we had a wrapper on free() that checked for NULL. I'll fix this one just to be consistent. > > Generally I find the per-command functions rather an improvement. > I did too. I tried to split this patch up into two parts, one that broke out the functions, and one that added if-then, and found that the first patch was just as unwieldily without the if-then stuff as with. > > 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... > I think one grand \if false \a \c some_connect_string ... \z some_table_name \endif should do the trick, but it wouldn't detect memory leaks. > > In "exec_command_connect" an argument is changed from "-reuse-previous" to > "-reuse-previous=", not sure why. > It shouldn't have been. Good catch. Most commands were able to be migrated with simple changes (status => *status, strcmp() if-block becomes active-if-block, etc), but that one was slightly different. > > 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? > Yes, and a few things like that, but I wanted this patch to keep as much code as-is as possible. > > 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'd be in favor of that as well > > I would suggest to put together all if-related backslash command, so that > the stack management is all in one function instead of 4. > I recognize the urge to group them together, but would there be any actual code sharing between them? Wouldn't I be either re-checking the string "cmd" again, or otherwise setting an enum that I immediately re-check inside the all_branching_commands() function? > > For pset the inactive branch does OT_NORMAL instead of OT_NOT_EVAL, not > sure why. An oversight. Good catch.
