This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v2 02/18] Remove completion_tracker_t from the public completion API.
- From: Doug Evans <dje at google dot com>
- To: Keith Seitz <keiths at redhat dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Thu, 21 May 2015 23:34:31 +0000
- Subject: Re: [PATCH v2 02/18] Remove completion_tracker_t from the public completion API.
- Authentication-results: sourceware.org; auth=none
Keith Seitz writes:
> This patch removes the recently introduced "completion tracker" concept
> from the public completion API. In the previous patch, the completion
> tracker was added to the (new) completer_data structure that is now
> used by all completion functions.
>
> Since completion_tracker_t is now used entirely inside completer.c, there
> is no need to make its type opaque, so the typedef has been removed.
>
> This patch also fixes gdb/17960, which is easily demonstrated:
>
> $ gdb -nx -q gdb
> (gdb) b gdb.c:ma<TAB>
> ./../src/gdb/completer.c:837: internal-error: maybe_add_completion:
> Assertion `tracker != NULL' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Quit this debugging session? (y or n)
>
> This occurs because the code path in this case is:
> complete_line
> - complete_line_internal
> -- location_completer
> --- make_file_symbol_completion_list
> (...)
> ---- completion_list_add_name
> ----- maybe_add_completion
> ----> gdb_assert (tracker != NULL);
>
> The tracker in maybe_add_completion is a static global in symtab.c called
> completion_tracker. It is initialized by
> default_make_symbol_completion_list_break_on_1 (which is a defaulted
> language vector method). In this case, this function is never called
> and completion_tracker is left NULL/uninitialized.
>
> gdb/ChangeLog
>
> PR gdb/17960
> * completer.c (struct completer_data) <tracker>: Change type
> to htab_t.
> (enum maybe_add_completion_enum): Moved here from completer.h.
> (DEFAULT_MAX_COMPLETIONS): Define.
> (max_completions): Initialize global to above value.
> (new_completion_tracker): Delete.
> (new_completer_data): New function.
> (make_cleanup_free_completion_tracker): Delete.
> (free_completer_data): New function.
> (maybe_add_completion): Make static.
> Add comments from completer.h.
> Change argument `tracker' to struct completer_data and use the
> tracker in that structure to do completion limiting.
> (add_completion): New function.
> (complete_line): Remove completion_tracker code.
> Allocate a completer_data and pass that to complete_line_internal.
> Remove now unused cleanup.
> When complete_line_internal returns more completions than are
> in the tracker, reset the tracker and use maybe_add_completion
> to implement completion tracking.
> Use add_completion instead of maybe_add_completion.
> (gdb_completion_word_break_characters): Use completer_data.
> * completer.h (completion_tracker_t): Remove.
> (new_completion_tracker): Remove.
> (make_cleanup_free_completion_tracker): Remove.
> (enum maybe_add_completion_enum): Move to completer.c.
> (maybe_add_completion): move to completer.c.
> (enum add_completion_status): Define.
> (add_completion): Declare.
> * symtab.c (completion_tracker): Remove global variable.
> (completion_list_add_name): Use add_completion instead of
> maybe_add_completion.
> (default_make_symbol_completion_list_break_on_1): Remove now
> unused cleanup.
> Do not allocate a completion tracker. Use the supplied
> completer_data instead.
>
> gdb/testsuite/ChangeLog
>
> PR gdb/17960
> * gdb.base/completion.exp: Add some basic tests for the
> location completer, including a regression test for
> the gdb/17960 assertion failure.
Hi.
A few high level questions here.
I don't want to impose a rewrite, I think(!) what I'm asking for
(assuming we're agreed we want it, which even I'm not sure of yet)
can be built on top of what you've got.
Plus a few usual nits. :-)
> ---
> gdb/completer.c | 203
+++++++++++++++++++++------------
> gdb/completer.h | 66 +++--------
> gdb/symtab.c | 36 ------
> gdb/testsuite/gdb.base/completion.exp | 78 +++++++++++++
> 4 files changed, 231 insertions(+), 152 deletions(-)
>
> diff --git a/gdb/completer.c b/gdb/completer.c
> index f2b31e9..085b003 100644
> --- a/gdb/completer.c
> +++ b/gdb/completer.c
> @@ -91,10 +91,31 @@ static char *gdb_completer_quote_characters = "'";
>
> struct completer_data
> {
> - /* The completion tracker being used by the completer. */
> - completion_tracker_t tracker;
> + /* A hashtable used to track completions. */
> + htab_t tracker;
> };
>
> +/* Return values for maybe_add_completion. */
> +
> +enum maybe_add_completion_enum
> +{
> + /* NAME has been recorded and max_completions has not been reached,
> + or completion tracking is disabled (max_completions < 0). */
> + MAYBE_ADD_COMPLETION_OK,
> +
> + /* NAME has been recorded and max_completions has been reached
> + (thus the caller can stop searching). */
> + MAYBE_ADD_COMPLETION_OK_MAX_REACHED,
> +
> + /* max-completions entries has been reached.
> + Whether NAME is a duplicate or not is not determined. */
> + MAYBE_ADD_COMPLETION_MAX_REACHED,
> +
> + /* NAME has already been recorded.
> + Note that this is never returned if completion tracking is disabled
> + (max_completions < 0). */
> + MAYBE_ADD_COMPLETION_DUPLICATE
> +};
>
> /* Accessor for some completer data that may interest other files. */
>
> @@ -802,47 +823,47 @@ complete_line_internal (struct completer_data
*cdata,
>
> /* See completer.h. */
>
> -int max_completions = 200;
> +#define DEFAULT_MAX_COMPLETIONS 200
> +int max_completions = DEFAULT_MAX_COMPLETIONS;
>
> -/* See completer.h. */
> +/* Allocate a new completer data structure. */
>
> -completion_tracker_t
> -new_completion_tracker (void)
> +static struct completer_data *
> +new_completer_data (int size)
> {
> - if (max_completions <= 0)
> - return NULL;
> + struct completer_data *cdata = XCNEW (struct completer_data);
>
> - return htab_create_alloc (max_completions,
> - htab_hash_string, (htab_eq) streq,
> - NULL, xcalloc, xfree);
> + cdata->tracker
> + = htab_create_alloc ((size < 0 ? DEFAULT_MAX_COMPLETIONS : size),
> + htab_hash_string, (htab_eq) streq, NULL,
> + xcalloc, xfree);
> + return cdata;
> }
>
> -/* Cleanup routine to free a completion tracker and reset the pointer
> - to NULL. */
> +/* Free the completion data represented by P. */
> +
> static void
> -free_completion_tracker (void *p)
> +free_completer_data (void *p)
> {
> - completion_tracker_t *tracker_ptr = p;
> + struct completer_data *cdata = p;
>
> - htab_delete (*tracker_ptr);
> - *tracker_ptr = NULL;
> + htab_delete (cdata->tracker);
> + xfree (cdata);
> }
>
> -/* See completer.h. */
> +/* Add the completion NAME to the list of generated completions if
> + it is not there already.
> + If max_completions is negative, nothing is done, not even watching
> + for duplicates, and MAYBE_ADD_COMPLETION_OK is always returned.
>
> -struct cleanup *
> -make_cleanup_free_completion_tracker (completion_tracker_t *tracker_ptr)
> -{
> - if (*tracker_ptr == NULL)
> - return make_cleanup (null_cleanup, NULL);
> + If MAYBE_ADD_COMPLETION_MAX_REACHED is returned, callers are
required to
> + record at least one more completion. The final list will be pruned
to
> + max_completions, but recording at least one more than
max_completions is
> + the signal to the completion machinery that too many completions were
> + found. */
The comment about adding one more is no longer valid, delete it.
[I haven't done so in the current tree to not interfere.]
>
> - return make_cleanup (free_completion_tracker, tracker_ptr);
> -}
> -
> -/* See completer.h. */
> -
> -enum maybe_add_completion_enum
> -maybe_add_completion (completion_tracker_t tracker, char *name)
> +static enum maybe_add_completion_enum
> +maybe_add_completion (struct completer_data *cdata, char *name)
> {
> void **slot;
>
> @@ -851,23 +872,49 @@ maybe_add_completion (completion_tracker_t
tracker, char *name)
> if (max_completions == 0)
> return MAYBE_ADD_COMPLETION_MAX_REACHED;
>
> - gdb_assert (tracker != NULL);
> -
> - if (htab_elements (tracker) >= max_completions)
> + if (htab_elements (cdata->tracker) >= max_completions)
> return MAYBE_ADD_COMPLETION_MAX_REACHED;
>
> - slot = htab_find_slot (tracker, name, INSERT);
> + slot = htab_find_slot (cdata->tracker, name, INSERT);
>
> if (*slot != HTAB_EMPTY_ENTRY)
> return MAYBE_ADD_COMPLETION_DUPLICATE;
>
> *slot = name;
>
> - return (htab_elements (tracker) < max_completions
> + return (htab_elements (cdata->tracker) < max_completions
> ? MAYBE_ADD_COMPLETION_OK
> : MAYBE_ADD_COMPLETION_OK_MAX_REACHED);
> }
>
> +/* See completer.h. */
> +
> +enum add_completion_status
> +add_completion (struct completer_data *cdata, VEC (char_ptr) **result,
> + char *name)
> +{
> + enum maybe_add_completion_enum add_status;
> +
Note to self: Would it be useful to do the xstrdup here?
We could even provide a second version that took a string and length
if it helped (have to audit all callers).
> + add_status = maybe_add_completion (cdata, name);
> + switch (add_status)
> + {
> + case MAYBE_ADD_COMPLETION_OK:
> + VEC_safe_push (char_ptr, *result, name);
> + break;
> + case MAYBE_ADD_COMPLETION_OK_MAX_REACHED:
> + VEC_safe_push (char_ptr, *result, name);
> + return ADD_COMPLETION_MAX_REACHED;
> + case MAYBE_ADD_COMPLETION_MAX_REACHED:
> + xfree (name);
> + return ADD_COMPLETION_MAX_REACHED;
> + case MAYBE_ADD_COMPLETION_DUPLICATE:
> + xfree (name);
> + break;
> + }
> +
> + return ADD_COMPLETION_OK;
> +}
> +
> void
> throw_max_completions_reached_error (void)
> {
> @@ -892,54 +939,63 @@ complete_line (const char *text, const char
*line_buffer, int point)
> {
> VEC (char_ptr) *list;
> VEC (char_ptr) *result = NULL;
> - struct cleanup *cleanups;
> - completion_tracker_t tracker;
> + struct cleanup *cdata_cleanup, *list_cleanup;
> char *candidate;
> - int ix, max_reached;
> + int ix;
> + struct completer_data *cdata;
>
> if (max_completions == 0)
> return NULL;
> - list = complete_line_internal (NULL, text, line_buffer, point,
> +
> + cdata = new_completer_data (max_completions);
> + cdata_cleanup = make_cleanup (free_completer_data, cdata);
> + list = complete_line_internal (cdata, text, line_buffer, point,
> handle_completions);
> if (max_completions < 0)
> - return list;
> -
> - tracker = new_completion_tracker ();
> - cleanups = make_cleanup_free_completion_tracker (&tracker);
> - make_cleanup_free_char_ptr_vec (list);
> -
> - /* Do a final test for too many completions. Individual completers
may
> - do some of this, but are not required to. Duplicates are also
removed
> - here. Otherwise the user is left scratching his/her head:
readline and
> - complete_command will remove duplicates, and if removal of
duplicates
> - there brings the total under max_completions the user may think
gdb quit
> - searching too early. */
> -
> - for (ix = 0, max_reached = 0;
> - !max_reached && VEC_iterate (char_ptr, list, ix, candidate);
> - ++ix)
> {
> - enum maybe_add_completion_enum add_status;
> + do_cleanups (cdata_cleanup);
> + return list;
> + }
> +
> + list_cleanup = make_cleanup_free_char_ptr_vec (list);
>
> - add_status = maybe_add_completion (tracker, candidate);
> + /* If complete_line_internal returned more completions than were
> + recorded by the completion tracker, then the completer function
that
> + was run does not support completion tracking. In this case,
> + do a final test for too many completions.
>
> - switch (add_status)
> + Duplicates are also removed here. Otherwise the user is left
> + scratching his/her head: readline and complete_command will remove
> + duplicates, and if removal of duplicates there brings the total
under
> + max_completions the user may think gdb quit searching too early.
*/
> +
If we still allow completers to not use the tracker machinery then
there's a u/i issue here: if less than the max is found then dups
aren't removed, whereas if >= max is found then dups are removed.
The difference in behaviour is odd.
Would it hurt to always perform this pass (IOW remove the "if") ?
OTOH, we *could* *require/expect* all completers to use the tracker
machinery.
In which case, do we need completers to return a vector of strings?
[ultimately we do, but internally we don't]
We could instead just use the hash table as the only repository of the
strings, and then create the vector of strings here from the hashtab.
In the case when completion limiting is turned off we don't want to
use the hashtab to record strings: we can still keep the list in cdata,
as a vector of strings (and leave the hashtab unused).
> + if (VEC_length (char_ptr, list) > htab_elements (cdata->tracker))
> + {
> + enum add_completion_status max_reached = ADD_COMPLETION_OK;
> +
> + /* Clear the tracker so that we can re-use it to count the number
> + of returned completions. */
> + htab_empty (cdata->tracker);
> +
> + for (ix = 0; (max_reached == ADD_COMPLETION_OK
> + && VEC_iterate (char_ptr, list, ix, candidate)); ++ix)
> {
> - case MAYBE_ADD_COMPLETION_OK:
> - VEC_safe_push (char_ptr, result, xstrdup (candidate));
> - break;
> - case MAYBE_ADD_COMPLETION_OK_MAX_REACHED:
> - VEC_safe_push (char_ptr, result, xstrdup (candidate));
> - max_reached = 1;
> - break;
> - case MAYBE_ADD_COMPLETION_MAX_REACHED:
> - gdb_assert_not_reached ("more than max completions reached");
> - case MAYBE_ADD_COMPLETION_DUPLICATE:
> - break;
> + max_reached = add_completion (cdata, &result, xstrdup (candidate));
> }
> +
> + /* The return result has been assembled and the original list from
> + complete_line_internal is no longer needed. Free it. */
> + do_cleanups (list_cleanup);
> + }
> + else
> + {
> + /* There is a valid tracker for the completion -- simply return
> + the completed list. */
> + discard_cleanups (list_cleanup);
> + result = list;
> }
>
> - do_cleanups (cleanups);
> + do_cleanups (cdata_cleanup);
>
> return result;
> }
> @@ -1032,9 +1088,14 @@ char *
> gdb_completion_word_break_characters (void)
> {
> VEC (char_ptr) *list;
> + struct completer_data *cdata;
> + struct cleanup *cleanup;
>
> - list = complete_line_internal (NULL, rl_line_buffer, rl_line_buffer,
> + cdata = new_completer_data (max_completions);
> + cleanup = make_cleanup (free_completer_data, cdata);
> + list = complete_line_internal (cdata, rl_line_buffer, rl_line_buffer,
> rl_point, handle_brkchars);
> + do_cleanups (cleanup);
> gdb_assert (list == NULL);
> return rl_completer_word_break_characters;
> }
> diff --git a/gdb/completer.h b/gdb/completer.h
> index f32c696..8b20ca5 100644
> --- a/gdb/completer.h
> +++ b/gdb/completer.h
> @@ -128,62 +128,32 @@ extern const char *skip_quoted (const char *);
>
> extern int max_completions;
>
> -/* Object to track how many unique completions have been generated.
> - Used to limit the size of generated completion lists. */
> +/* Return values for add_completion. */
>
> -typedef htab_t completion_tracker_t;
> -
> -/* Create a new completion tracker.
> - The result is a hash table to track added completions, or NULL
> - if max_completions <= 0. If max_completions < 0, tracking is
disabled.
> - If max_completions == 0, the max is indeed zero. */
> -
> -extern completion_tracker_t new_completion_tracker (void);
> -
> -/* Make a cleanup to free a completion tracker, and reset its pointer
> - to NULL. */
> -
> -extern struct cleanup *make_cleanup_free_completion_tracker
> - (completion_tracker_t *tracker_ptr);
> -
> -/* Return values for maybe_add_completion. */
> -
> -enum maybe_add_completion_enum
> +enum add_completion_status
> {
> - /* NAME has been recorded and max_completions has not been reached,
> - or completion tracking is disabled (max_completions < 0). */
> - MAYBE_ADD_COMPLETION_OK,
> -
> - /* NAME has been recorded and max_completions has been reached
> - (thus the caller can stop searching). */
> - MAYBE_ADD_COMPLETION_OK_MAX_REACHED,
> -
> - /* max-completions entries has been reached.
> - Whether NAME is a duplicate or not is not determined. */
> - MAYBE_ADD_COMPLETION_MAX_REACHED,
> -
> - /* NAME has already been recorded.
> - Note that this is never returned if completion tracking is disabled
> - (max_completions < 0). */
> - MAYBE_ADD_COMPLETION_DUPLICATE
> + /* Completion was added -- keep looking for more. */
> + ADD_COMPLETION_OK,
> +
> + /* Maximum number of completions has been reached or the maximum
> + has already been reached by a previous call to add_completion.
> + Callers can make no assumptions about whether the completion was
> + added or not. */
> + ADD_COMPLETION_MAX_REACHED
> };
Given that there's only two possible return codes in the public API,
do we need an enum? I'm ok with it, just a thought.
>
> -/* Add the completion NAME to the list of generated completions if
> - it is not there already.
> - If max_completions is negative, nothing is done, not even watching
> - for duplicates, and MAYBE_ADD_COMPLETION_OK is always returned.
> +/* Add the given NAME to the completer result in CDATA.
> + Returns one of the above enum add_completion_status codes to indicate
> + whether the caller should continue to look for/compute more
completions.
>
> - If MAYBE_ADD_COMPLETION_MAX_REACHED is returned, callers are
required to
> - record at least one more completion. The final list will be pruned
to
> - max_completions, but recording at least one more than
max_completions is
> - the signal to the completion machinery that too many completions were
> - found. */
> + NAME should be malloc'd by the caller. That memory is now
> + under control of the completer and should not be freed by the
caller. */
Note to self: What's the cost of removing the "caller must xstrdup"
requirement?
>
> -extern enum maybe_add_completion_enum
> - maybe_add_completion (completion_tracker_t tracker, char *name);
> +extern enum add_completion_status
> + add_completion (struct completer_data *cdata,
> + VEC (char_ptr) **result, char *name);
>
> /* Wrapper to throw MAX_COMPLETIONS_REACHED_ERROR. */
>
> extern void throw_max_completions_reached_error (void);
> -
keep this blank line
> #endif /* defined (COMPLETER_H) */
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index c0562e1..eb96d52 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -5017,15 +5017,6 @@ static VEC (char_ptr) *return_val;
> completion_list_add_name \
> (cdata, MSYMBOL_NATURAL_NAME (symbol), (sym_text), (len), (text),
(word))
>
> -/* Tracker for how many unique completions have been generated. Used
> - to terminate completion list generation early if the list has grown
> - to a size so large as to be useless. This helps avoid GDB seeming
> - to lock up in the event the user requests to complete on something
> - vague that necessitates the time consuming expansion of many symbol
> - tables. */
> -
> -static completion_tracker_t completion_tracker;
> -
> /* Test to see if the symbol specified by SYMNAME (which is already
> demangled for C++ symbols) matches SYM_TEXT in the first SYM_TEXT_LEN
> characters. If so, add it to the current completion list. */
> @@ -5045,7 +5036,6 @@ completion_list_add_name (struct completer_data
*cdata,
>
> {
> char *newobj;
> - enum maybe_add_completion_enum add_status;
>
> if (word == sym_text)
> {
> @@ -5067,23 +5057,9 @@ completion_list_add_name (struct completer_data
*cdata,
> strcat (newobj, symname);
> }
>
> - add_status = maybe_add_completion (completion_tracker, newobj);
> -
> - switch (add_status)
> - {
> - case MAYBE_ADD_COMPLETION_OK:
> - VEC_safe_push (char_ptr, return_val, newobj);
> - break;
> - case MAYBE_ADD_COMPLETION_OK_MAX_REACHED:
> - VEC_safe_push (char_ptr, return_val, newobj);
> - throw_max_completions_reached_error ();
> - case MAYBE_ADD_COMPLETION_MAX_REACHED:
> - xfree (newobj);
> - throw_max_completions_reached_error ();
> - case MAYBE_ADD_COMPLETION_DUPLICATE:
> - xfree (newobj);
> - break;
> - }
> + if (add_completion (cdata, &return_val, newobj)
> + == ADD_COMPLETION_MAX_REACHED)
> + throw_max_completions_reached_error ();
> }
> }
>
> @@ -5329,7 +5305,6 @@ default_make_symbol_completion_list_break_on_1
(struct completer_data *cdata,
> /* Length of sym_text. */
> int sym_text_len;
> struct add_name_data datum;
> - struct cleanup *cleanups;
>
> /* Now look for the symbol we are supposed to complete on. */
> {
> @@ -5400,9 +5375,6 @@ default_make_symbol_completion_list_break_on_1
(struct completer_data *cdata,
> }
> gdb_assert (sym_text[sym_text_len] == '\0' || sym_text[sym_text_len]
== '(');
>
> - completion_tracker = new_completion_tracker ();
> - cleanups = make_cleanup_free_completion_tracker (&completion_tracker);
> -
> datum.sym_text = sym_text;
> datum.sym_text_len = sym_text_len;
> datum.text = text;
> @@ -5520,8 +5492,6 @@ default_make_symbol_completion_list_break_on_1
(struct completer_data *cdata,
> /* User-defined macros are always visible. */
> macro_for_each (macro_user_macros, add_macro_name, &datum);
> }
> -
> - do_cleanups (cleanups);
> }
>
> VEC (char_ptr) *
> diff --git a/gdb/testsuite/gdb.base/completion.exp
b/gdb/testsuite/gdb.base/completion.exp
> index f77bfe2..5afd851 100644
> --- a/gdb/testsuite/gdb.base/completion.exp
> +++ b/gdb/testsuite/gdb.base/completion.exp
> @@ -777,6 +777,84 @@ gdb_test_multiple "" "$test" {
> }
>
> #
> +# Tests for the location completer
> +#
> +
> +# Turn off pending breakpoint support so that we don't get queried
> +# all the time.
> +gdb_test_no_output "set breakpoint pending off"
> +
> +set subsrc [string range $srcfile 0 [expr {[string length $srcfile] -
3}]]
> +set test "tab complete break $subsrc"
> +send_gdb "break $subsrc\t\t"
> +gdb_test_multiple "" $test {
> + -re "break\.c.*break1\.c.*$gdb_prompt " {
> + send_gdb "1\t\n"
> + gdb_test_multiple "" $test {
> + -re ".*Function \"$srcfile2\" not defined\..*$gdb_prompt " {
> + pass $test
> + }
> + -re "$gdb_prompt p$" {
> + fail $test
> + }
> + }
> + }
> +
> + -re "$gdb_prompt p$" {
> + fail $test
> + }
> +}
> +
> +gdb_test "complete break $subsrc" "break\.c.*break1\.c"
> +
> +# gdb/17960
> +set test "tab complete break $srcfile:ma"
> +send_gdb "break $srcfile:ma\t"
> +gdb_test_multiple "" $test {
> + -re "break $srcfile:main " {
> + send_gdb "\n"
> + gdb_test_multiple "" $test {
> + -re ".*Breakpoint.*at .*/$srcfile, line .*$gdb_prompt " {
> + pass $test
> + gdb_test_no_output "delete breakpoint \$bpnum" \
> + "delete breakpoint for $test"
> + }
> + -re "$gdb_prompt p$" {
> + fail $test
> + }
> + }
> + }
> + -re "$gdb_prompt p$" {
> + fail $test
> + }
> +}
> +
> +gdb_test "complete break $srcfile:ma" "break\.c:main"
> +
> +set test "tab complete break need"
> +send_gdb "break need\t"
> +gdb_test_multiple "" $test {
> + -re "break need_malloc " {
> + send_gdb "\n"
> + gdb_test_multiple "" $test {
> + -re ".*Breakpoint.*at .*/$srcfile, line .*$gdb_prompt " {
> + pass $test
> + gdb_test_no_output "delete breakpoint \$bpnum" \
> + "delete breakpoint for $test"
> + }
> + -re "$gdb_prompt p$" {
> + fail $test
> + }
> + }
> + }
> + -re "$gdb_prompt p$" {
> + fail $test
> + }
> +}
> +
> +gdb_test "complete break need" "need_malloc"
> +
> +#
> # Completion limiting.
> #
>
>
I see a pattern in several(all?) completers.
This is complete_on_cmdlist:
match = (char *) xmalloc (strlen (word) + strlen (ptr->name) + 1);
if (word == text)
strcpy (match, ptr->name);
else if (word > text)
{
/* Return some portion of ptr->name. */
strcpy (match, ptr->name + (word - text));
}
else
{
/* Return some of text plus ptr->name. */
strncpy (match, word, text - word);
match[text - word] = '\0';
strcat (match, ptr->name);
}
This is completion_list_add_name:
if (word == sym_text)
{
newobj = xmalloc (strlen (symname) + 5);
strcpy (newobj, symname);
}
else if (word > sym_text)
{
/* Return some portion of symname. */
newobj = xmalloc (strlen (symname) + 5);
strcpy (newobj, symname + (word - sym_text));
}
else
{
/* Return some of SYM_TEXT plus symname. */
newobj = xmalloc (strlen (symname) + (sym_text - word) + 5);
strncpy (newobj, word, sym_text - word);
newobj[sym_text - word] = '\0';
strcat (newobj, symname);
}
This is add_filename_to_list:
if (word == text)
{
/* Return exactly fname. */
newobj = xmalloc (fnlen + 5);
strcpy (newobj, fname);
}
else if (word > text)
{
/* Return some portion of fname. */
newobj = xmalloc (fnlen + 5);
strcpy (newobj, fname + (word - text));
}
else
{
/* Return some of TEXT plus fname. */
newobj = xmalloc (fnlen + (text - word) + 5);
strncpy (newobj, word, text - word);
newobj[text - word] = '\0';
strcat (newobj, fname);
}
This is ada-lang.c:symbol_completion_add:
if (word == orig_text)
{
completion = xmalloc (strlen (match) + 5);
strcpy (completion, match);
}
else if (word > orig_text)
{
/* Return some portion of sym_name. */
completion = xmalloc (strlen (match) + 5);
strcpy (completion, match + (word - orig_text));
}
else
{
/* Return some of ORIG_TEXT plus sym_name. */
completion = xmalloc (strlen (match) + (orig_text - word) + 5);
strncpy (completion, word, orig_text - word);
completion[orig_text - word] = '\0';
strcat (completion, match);
}
There are more examples.
If we're going to do a fullscale completion API cleanup we should
at least think about how we'd address this (IOW simplify it and remove
all the duplication: it's excessive and warrants attention).
We don't have to implement it in this pass, just understand what we
want to do (if anything).
Seems like all of this logic could be collapsed into a function
that then calls add_completion or some such.