Re: psql tab completion enhancements - Mailing list pgsql-patches

A few minor stylistic gripes...

On Fri, 2006-01-06 at 20:27 +0100, Joachim Wieland wrote:
> *** 150,155 ****
> --- 151,171 ----
>   do {completion_charp = Query_for_list_of_attributes;
> completion_info_charp = table; matches = completion_matches(text,
> complete_from_query); } while(0)
>
>   /*
> +  * Keep the "malloced" keyword in all the names such that we
> remember that
> +  * memory got allocated here. COMPLETE_WITH_MALLOCED_LIST frees this
> memory.
> +  */
> + #define COMPLETE_WITH_MALLOCED_LIST(list) \
> +       do { COMPLETE_WITH_LIST((const char**) list); free(list); list
> = (char**) 0; } while(0)

NULL is better style than 0 in a pointer context. Also, why are the
casts necessary?

>   /* Forward declaration of functions */
> + static char **get_empty_list();

Should be "static char **get_empty_list(void);", as C89 doesn't check
the parameters passed to a function declared with an empty parameter
list.

> *** 754,760 ****
>         else if (pg_strcasecmp(prev3_wd, "TABLE") == 0 &&
>                          (pg_strcasecmp(prev_wd, "ALTER") == 0 ||
>                           pg_strcasecmp(prev_wd, "RENAME") == 0))
> !               COMPLETE_WITH_ATTR(prev2_wd);
>
>         /* ALTER TABLE xxx RENAME yyy */
>         else if (pg_strcasecmp(prev4_wd, "TABLE") == 0 &&
> --- 780,796 ----
>         else if (pg_strcasecmp(prev3_wd, "TABLE") == 0 &&
>                          (pg_strcasecmp(prev_wd, "ALTER") == 0 ||
>                           pg_strcasecmp(prev_wd, "RENAME") == 0))
> !       {
> !               char** list = GET_MALLOCED_LIST_WITH_ATTR(prev2_wd);

Should be "char **list = ..." -- similarly in several other places.

> *** 1454,1461 ****
>         else if (pg_strcasecmp(prev_wd, "LOCK") == 0 ||
>                          (pg_strcasecmp(prev_wd, "TABLE") == 0 &&
>                           pg_strcasecmp(prev2_wd, "LOCK") == 0))
> !               COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables,
> NULL);
> !
>         /* For the following, handle the case of a single table only
> for now */
>
>         /* Complete LOCK [TABLE] <table> with "IN" */
> --- 1497,1510 ----
>         else if (pg_strcasecmp(prev_wd, "LOCK") == 0 ||
>                          (pg_strcasecmp(prev_wd, "TABLE") == 0 &&
>                           pg_strcasecmp(prev2_wd, "LOCK") == 0))
> !       {
> !               char** list =
> GET_MALLOCED_LIST_WITH_SCHEMA_QUERY(Query_for_list_of_tables, NULL);
> !               /* if we have only seen LOCK but not LOCK TABLE so
> far, offer
> !                * the TABLE keyword as well */

Is this actually useful? The "TABLE" keyword is just noise.

> *** 2315,2320 ****
> --- 2368,2437 ----
>
>   }
>
> + /* LIST HELPER FUNCTIONS */
> +
> + static char**
> + get_empty_list() {
> +       char** list = malloc(sizeof(char*));
> +       list[0] = NULL;
> +       return list;
> + }

Brace style ("{" on newline), "char **", and "(void"), as above.

> + static char**
> + get_query_list(const char *text,
> +                          const char *query,
> +                          const char* completion_info)
> + {
> +       return _get_query_list(0, text, query, completion_info);
> + }

The difference between get_query_list() and _get_query_list() is not
reflected in the names of the methods. An "_internal" suffix would be
better, for example.

> + static char**
> + _get_query_list(int is_schema_query,
> +                               const char *text,
> +                               const char *query,
> +                               const char* completion_info)

"bool" for boolean variables rather than "int", and consistent parameter
declarations ("char *" not "char*").

> + static char**
> + list_add_item(char **list, char *item)
> + {
> +       int size = 0;
> +       while (list[size])
> +               size++;
> +       list = realloc(list, sizeof(char*) * (size + 1 + 1));
> +       list[size] = item;
> +       list[size+1] = NULL;
> +       return list;
> + }

That's a terribly inefficient implementation.

-Neil