Re: Making tab-complete.c easier to maintain

On Wed, Dec 9, 2015 at 8:17 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> On Mon, Dec 7, 2015 at 8:41 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> 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.
>
> Thanks for looking at this Michael.  It's probably not much fun to
> review!  Here is a new version with that bit removed.  More responses
> inline below.

I had a hard time not sleeping when reading it... That's very mechanical.

> I agree that would probably be better but Alvaro suggested following
> the existing logic in the first pass, which was mostly based on tails,
> and then considering simpler head-based patterns in a future pass.

Fine with me.

So what do we do now? There is your patch, which is already quite big,
but as well a second patch based on regexps, which is far bigger. And
at the end they provide a similar result:

Here is for example what the regexp patch does for some complex
checks, like ALTER TABLE RENAME:    /* ALTER TABLE xxx RENAME yyy */
-    else if (pg_strcasecmp(prev4_wd, "TABLE") == 0 &&
-             pg_strcasecmp(prev2_wd, "RENAME") == 0 &&
-             pg_strcasecmp(prev_wd, "CONSTRAINT") != 0 &&
-             pg_strcasecmp(prev_wd, "TO") != 0)
+    else if (MATCH("TABLE #id RENAME !CONSTRAINT|TO"))

And what Thomas's patch does:
+    else if (TailMatches5("ALTER", "TABLE", MatchAny, "RENAME", MatchAny) &&
+             !TailMatches1("CONSTRAINT|TO"))

The regexp patch makes the negative checks somewhat easier to read
(there are 19 positions in tab-complete.c doing that), still inventing
a new langage and having a heavy refactoring just tab completion of
psql seems a bit too much IMO, so my heart balances in favor of
Thomas' stuff. Thoughts from others?
-- 
Michael