Re: Making tab-complete.c easier to maintain

On Tue, Nov 17, 2015 at 12:19 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
wrote:
> Thomas Munro wrote:
>> New version attached, merging recent changes.
>
> I wonder about the TailMatches and Matches macros --- wouldn't it be
> better to have a single one, renaming TailMatches to Matches and
> replacing the current Matches() with an initial token that corresponds
> to anchoring to start of command? Just wondering, not terribly attached
> to the idea.

+ /* TODO:TM -- begin temporary, not part of the patch! */
+ Assert(!word_matches(NULL, ""));
+ [...]
+ Assert(!word_matches("foo", ""));
+ /* TODO:TM -- end temporary */

Be sure to not forget to remove that later.

- else if (pg_strcasecmp(prev5_wd, "DEFAULT") == 0 &&
- pg_strcasecmp(prev4_wd, "PRIVILEGES") == 0 &&
- (pg_strcasecmp(prev3_wd, "FOR") == 0 ||
- pg_strcasecmp(prev3_wd, "IN") == 0))
- {
- static const char *const
list_ALTER_DEFAULT_PRIVILEGES_REST[] =
- {"GRANT", "REVOKE", NULL};
-
- COMPLETE_WITH_LIST(list_ALTER_DEFAULT_PRIVILEGES_REST);
- }
+ else if (TailMatches5("DEFAULT", "PRIVILEGES", "FOR", "ROLE",
MatchAny) ||
+ TailMatches5("DEFAULT", "PRIVILEGES", "IN",
"SCHEMA", MatchAny))
+ CompleteWithList2("GRANT", "REVOKE");
For this chunk I think that you need to check for ROLE|USER and not only
ROLE.

+ else if (TailMatches4("ALTER", "DOMAIN", MatchAny, "RENAME"))
{
static const char *const list_ALTERDOMAIN[] =
{"CONSTRAINT", "TO", NULL};
I think you should remove COMPLETE_WITH_LIST here for consistency with the
rest.

- else if (pg_strcasecmp(prev5_wd, "DOMAIN") == 0 &&
- pg_strcasecmp(prev3_wd, "RENAME") == 0 &&
- pg_strcasecmp(prev2_wd, "CONSTRAINT") == 0)
+ else if (TailMatches3("RENAME", "CONSTRAINT", MatchAny))
COMPLETE_WITH_CONST("TO");
Perhaps you are missing DOMAIN here?

- else if (pg_strcasecmp(prev4_wd, "ALTER") == 0 &&
- pg_strcasecmp(prev3_wd, "SEQUENCE") == 0 &&
- pg_strcasecmp(prev_wd, "NO") == 0)
- {
- static const char *const list_ALTERSEQUENCE2[] =
- {"MINVALUE", "MAXVALUE", "CYCLE", NULL};
-
- COMPLETE_WITH_LIST(list_ALTERSEQUENCE2);
- }
+ else if (TailMatches4("ALTER", "SEQUEMCE", MatchAny, "NO"))
+ CompleteWithList3("MINVALUE", "MAXVALUE", "CYCLE");
Typo here: s/SEQUEMCE/SEQUENCE.

- else if (pg_strcasecmp(prev5_wd, "TABLE") == 0 &&
- pg_strcasecmp(prev3_wd, "RENAME") == 0 &&
- (pg_strcasecmp(prev2_wd, "COLUMN") == 0 ||
- pg_strcasecmp(prev2_wd, "CONSTRAINT") == 0) &&
- pg_strcasecmp(prev_wd, "TO") != 0)
+ else if (TailMatches6("ALTER", "TABLE", MatchAny, "RENAME",
"COLUMN|CONSTRAINT", MatchAny) &&
+ !TailMatches1("TO"))
This should use TailMatches5 without ALTER for consistency with the
existing code?

- else if (pg_strcasecmp(prev_wd, "CLUSTER") == 0 &&
- pg_strcasecmp(prev2_wd, "WITHOUT") != 0)
+ else if (TailMatches1("CLUSTER") && !TailMatches2("WITHOUT",
"CLUSTER"))
Here removing CLUSTER should be fine.

- else if (pg_strcasecmp(prev2_wd, "CLUSTER") == 0 &&
- pg_strcasecmp(prev_wd, "ON") != 0 &&
- pg_strcasecmp(prev_wd, "VERBOSE") != 0)
- {
+ else if (TailMatches2("CLUSTER", MatchAny) &&
!TailMatches1("VERBOSE"))
Handling of ON has been forgotten.

- else if (pg_strcasecmp(prev3_wd, "CREATE") == 0 &&
- !(pg_strcasecmp(prev2_wd, "USER") == 0 &&
pg_strcasecmp(prev_wd, "MAPPING") == 0) &&
- (pg_strcasecmp(prev2_wd, "ROLE") == 0 ||
- pg_strcasecmp(prev2_wd, "GROUP") == 0 ||
pg_strcasecmp(prev2_wd, "USER") == 0))
+ else if (TailMatches3("CREATE", "ROLE|GROUP|USER", MatchAny) &&
+ !TailMatches3("CREATE", "USER", "MAPPING"))
!TailMatches2("USER", "MAPPING")?

- else if (pg_strcasecmp(prev4_wd, "CREATE") == 0 &&
- pg_strcasecmp(prev3_wd, "MATERIALIZED") == 0 &&
- pg_strcasecmp(prev2_wd, "VIEW") == 0)
+ else if (TailMatches3("CREATE", "MATERIALIZED", "VIEW"))
Forgot a MatchAny here?

- else if (pg_strcasecmp(prev_wd, "DELETE") == 0 &&
- !(pg_strcasecmp(prev2_wd, "ON") == 0 ||
- pg_strcasecmp(prev2_wd, "GRANT") == 0 ||
- pg_strcasecmp(prev2_wd, "BEFORE") == 0 ||
- pg_strcasecmp(prev2_wd, "AFTER") == 0))
+ else if (TailMatches1("DELETE") &&
!TailMatches2("ON|GRANT|BEFORE|AFTER", "DELETE"))
COMPLETE_WITH_CONST("FROM");
In the second clause checking for DELETE is not necessary.

- else if (pg_strcasecmp(prev_wd, "EXECUTE") == 0 &&
- prev2_wd[0] == '\0')
+ else if (Matches1("EXECUTE"))
This looks not complete.

+ else if (TailMatches1("EXPLAIN"))
+ CompleteWithList7("SELECT", "INSERT", "DELETE", "UPDATE",
"DECLARE",
+ "ANALYZE",
"VERBOSE");
+ else if (TailMatches2("EXPLAIN", "ANALYZE|ANALYSE"))
ANALYSE should be removed, former code only checked for ANALYZE => I heard
about the grammar issues here :)

- else if (pg_strcasecmp(prev4_wd, "GRANT") == 0)
+ else if (TailMatches4("GRANT", MatchAny, MatchAny,
MatchAny))
+ COMPLETE_WITH_CONST("TO");
+ else
+ COMPLETE_WITH_CONST("FROM");
+ }
+
+ /* Complete "GRANT/REVOKE * ON * *" with TO/FROM */
+ else if (TailMatches5("GRANT|REVOKE", MatchAny, "ON", MatchAny,
MatchAny))
+ {
+ if (TailMatches5("GRANT", MatchAny, MatchAny, MatchAny,
MatchAny))
Isn't the first check with TailMatches4 enough here?

- if (pg_strcasecmp(prev6_wd, "GRANT") == 0)
+ if (TailMatches6("GRANT", MatchAny, MatchAny, MatchAny,
MatchAny, MatchAny))
HeadMatches1 perhaps?
--
Michael