On Wed, Feb 22, 2017 at 4:00 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> Corey Huinker <corey.huin...@gmail.com> writes: > >> but if you think that it should be somewhere else, please advise Corey > >> about where to put it. > > > Just a reminder that I'm standing by for advice. > > Sorry, I'd lost track of this thread. > Judging by the volume of the 50-or-so active threads on this list, I figured you were busy. > > The issue at hand is whether the if-state should be a part of the > > PsqlScanState, or if it should be a separate state variable owned by > > MainLoop() and passed to HandleSlashCommands(), ... or some other > solution. > > My reaction to putting it in PsqlScanState is pretty much "over my dead > body". That's just trashing any pretense of an arms-length separation > between psql and the lexer proper. We only recently got done sweating > blood to create that separation, why would we toss it away already? > Good to know that history. > > If you're concerned about the notational overhead of passing two arguments > rather than one, my druthers would be to invent a new struct type, perhaps > named along the lines of PsqlFileState or PsqlInputState, and pass that > around. One of its fields would be a PsqlScanState pointer, the rest > would be for if-state and whatever else we think we need in per-input-file > state. > I wasn't, my reviewer was. I thought about the super-state structure like you described, and decided I was happy with two state params. > However, that way is doubling down on the assumption that the if-state is > exactly one-to-one with input file levels, isn't it? We might be better > off to just live with the separate arguments to preserve some flexibility > in that regard. The v12 patch doesn't look that awful in terms of what > it's adding to argument lists. > The rationale for tying if-state to file levels is not so much of anything if-then related, but rather of the mess we'd create for whatever poor soul decided to undertake \while loops down the road, and the difficulties they'd have trying to unwind/rewind jump points in file(s)...keeping it inside one file makes things simpler for the coding and the coder. > One thing I'm wondering is why the "active_branch" bool is in "pset" > and not in the conditional stack. That seems, at best, pretty grotty. > _psqlSettings is meant for reasonably persistent state. > With the if-stack moved to MainLoop(), nearly all the active_branch checks could be against a variable that lives in MainLoop(), with two big exceptions: GetVariable() needs to know when NOT to expand a variable because it's in a false-block, and get_prompt will need to know when it's in a false block for printing the '@' prompt hint or equivalent, and pset is the only global around I know of to do that. I can move nearly all the is-this-branch-active checks to structures inside of MainLoop(), and that does strike me as cleaner, but there will still have to be that gross bit where we update pset.active_branch so that the prompt and GetVariable() are clued in.
