Re: psql tab completion enhancements

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