Re: Making tab-complete.c easier to maintain

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
> On Sat, Dec 19, 2015 at 5:42 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> 1. It seems inconsistent that all the new macros are named in CamelCase
>> style, whereas there is still plenty of usage of the existing macros like
>> COMPLETE_WITH_LIST. It looks pretty jarring IMO. I think we should
>> either rename the new macros back to all-upper-case style, or rename the
>> existing macros in CamelCase style.
>>
>> I slightly favor the latter option; we're already pretty much breaking any
>> hope of tab-complete fixes applying backwards over this patch, so changing
>> the code even more doesn't seem like a problem. Either way, it's a quick
>> search-and-replace. Thoughts?

> Both would be fine, honestly. Now if we look at the current code there
> are already a lot of macros IN_UPPER_CASE, so it would make more sense
> on the contrary to have MATCHES_N and MATCHES_EXCEPT?

After some contemplation I decided that what was bugging me was mainly
the inconsistency of having some completion actions in camelcase and
immediately adjacent actions in all-upper-case. Making the test macros
camelcase isn't such a problem as long as they all look similar, and
I think it's more readable anyway. So I changed CompleteWithListN to
COMPLETE_WITH_LISTN and otherwise left the naming alone.

I've committed this now with a number of changes, many of them just
stylistic. Notable is that I rewrote word_matches to rely on
pg_strncasecmp rather than using toupper/tolower directly, so as to avoid
any possible change in semantics. (The code as submitted was flat wrong
anyway, since it wasn't being careful about passing only unsigned chars to
those functions.) I also got rid of its inconsistent treatment of empty
strings, in favor of not ever calling word_matches() on nonexistent words
in the first place. That just requires testing previous_words_count in
the TailMatches macros. I think it'd now be possible for
get_previous_words to skip generating empty strings for word positions
before the start of line, but have not experimented.

I found a couple more small errors in the converted match rules too,
but I have to admit my eyes started to glaze over while looking through
them. It's possible there are some remaining errors there. On the
other hand, it's at least as likely that we've gotten rid of some bugs.

There's a good deal more that could be done here:

1. I think it would be a good idea to convert the matching rules for
backslash commands too. To do that, we'd need to provide a case-sensitive
equivalent to word_match and the matching macros. I think we'd also have
to extend word_match to allow a trailing wildcard character, maybe "*".

2. I believe that a very large fraction of the TailMatches() rules really
ought to be Matches(), ie, they should not consider matches that don't
start at the start of the line. And there's another bunch that could
be Matches() if the author hadn't been unaccountably lazy about checking
all words of the expected command. If we converted as much as we could
that way, it would make psql_completion faster because many inapplicable
rules could be discarded after a single integer comparison on
previous_words_count, and it would greatly reduce the risk of inapplicable
matches. We can't do that for rules meant to apply to DML statements,
since they can be buried in WITH, EXPLAIN, etc ... but an awful lot of
the DDL rules could be changed.

3. The HeadMatches macros are pretty iffy because they can only look back
nine words. I'm tempted to redesign get_previous_words so it just
tokenizes the whole line rather than having an arbitrary limitation.
(For that matter, it's long overdue for it to be able to deal with
multiline input...)

I might go look at #3, but I can't currently summon the energy to tackle
#1 or #2 --- any volunteers?

regards, tom lane