Re: Making tab-complete.c easier to maintain

On Tue, Nov 17, 2015 at 12:19 AM, Alvaro Herrera <alvherre@2ndquadrant.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))

HeadMatches1 perhaps?

--
Michael