Re: Making tab-complete.c easier to maintain - Mailing list pgsql-hackers
| From | Thomas Munro |
|---|---|
| Subject | Re: Making tab-complete.c easier to maintain |
| Date | |
| Msg-id | CAEepm=3q9oBsd65F3AMTKw9-Rs+E+SDV3HpGV5yLj7ySetj7Yw@mail.gmail.com Whole thread Raw |
| In response to | Re: Making tab-complete.c easier to maintain (Alvaro Herrera <alvherre@2ndquadrant.com>) |
| Responses |
Re: Making tab-complete.c easier to maintain
|
| List | pgsql-hackers |
On Tue, Sep 8, 2015 at 1:56 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
+1
MatchAny would make more sense to me.
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
http://www.enterprisedb.com
Attachment
pgsql-hackers by date: