Re: Making tab-complete.c easier to maintain

On Tue, Sep 8, 2015 at 1:56 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
wrote:

> Thomas Munro wrote:
>
> > Thanks, good point. Here's a version that uses NULL via a macro ANY.
> > Aside from a few corrections it also now distinguishes between
> > TAIL_MATCHESn (common) and MATCHESn (rarely used for now), for example:
>
> This looks pretty neat -- 100x neater than what we have, at any rate. I
> would use your new MATCHESn() macros a bit more -- for instance the
> completion for "ALTER but not ALTER after ALTER TABLE" could be
> rephrased as simply MATCHES1("ALTER"), i.e. have it match at start of
> command only. Maybe that's just a matter of going over the new code
> after the initial run, so that we can have a first patch that's mostly
> mechanical and a second pass in which more semantically relevant changes
> are applied. Seems easier to review ...
>

+1

> I would use "ANY" as a keyword here. Sounds way too generic to me.
> Maybe "CompleteAny" or something like that.
>

MatchAny would make more sense to me.

Stylistically, I find there's too much uppercasing here. Maybe rename the
> macros like this instead:
>
> > + else if (TailMatches4("ALL", "IN", "TABLESPACE", ANY))
> > + CompleteWithList2("SET TABLESPACE", "OWNED BY");
>
> Not totally sure about this part TBH.
>

Ok, here's a rebased version that uses the style you suggested. It also
adds HeadMatchesN macros, so we can do this:

* Complete "GRANT/REVOKE ... TO/FROM" with username, PUBLIC,
* CURRENT_USER, or SESSION_USER.
*/
- else if (((pg_strcasecmp(prev9_wd, "GRANT") == 0 ||
- pg_strcasecmp(prev8_wd, "GRANT") == 0 ||
- pg_strcasecmp(prev7_wd, "GRANT") == 0 ||
- pg_strcasecmp(prev6_wd, "GRANT") == 0 ||
- pg_strcasecmp(prev5_wd, "GRANT") == 0) &&
- pg_strcasecmp(prev_wd, "TO") == 0) ||
- ((pg_strcasecmp(prev9_wd, "REVOKE") == 0 ||
- pg_strcasecmp(prev8_wd, "REVOKE") == 0 ||
- pg_strcasecmp(prev7_wd, "REVOKE") == 0 ||
- pg_strcasecmp(prev6_wd, "REVOKE") == 0 ||
- pg_strcasecmp(prev5_wd, "REVOKE") == 0) &&
- pg_strcasecmp(prev_wd, "FROM") == 0))
+ else if ((HeadMatches1("GRANT") && TailMatches1("TO")) ||
+ (HeadMatches1("REVOKE") && TailMatches1("FROM")))
COMPLETE_WITH_QUERY(Query_for_list_of_grant_roles);

So to recap:

MatchesN(...) -- matches the whole expression (up to lookback buffer size)
HeadMatchesN(...) -- matches the start of the expression (ditto)
TailMatchesN(...) -- matches the end of the expression
MatchAny -- placeholder

It would be nice to get rid of those numbers in the macro names, and I
understand that we can't use variadic macros. Should we use varargs
functions instead of macros? Then we could lose the numbers, but we'd need
to introduce global variables to keep the notation short and sweet (or pass
in the previous_words and previous_words_count, which would be ugly
boilerplate worse than the numbers).

--
Thomas Munro
http://www.enterprisedb.com