This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v3 7/9] Explicit locations: add UI features for CLI
- From: Keith Seitz <keiths at redhat dot com>
- To: Doug Evans <xdje42 at gmail dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Thu, 07 May 2015 10:07:28 -0700
- Subject: Re: [PATCH v3 7/9] Explicit locations: add UI features for CLI
- Authentication-results: sourceware.org; auth=none
- References: <20150217220619 dot 1312 dot 39861 dot stgit at valrhona dot uglyboxes dot com> <20150217220653 dot 1312 dot 96831 dot stgit at valrhona dot uglyboxes dot com> <m3mw3wju91 dot fsf at sspiff dot org>
On 03/01/2015 01:14 PM, Doug Evans wrote:
> Keith Seitz <keiths@redhat.com> writes:
>
>> +/* A helper function to collect explicit location matches for the given
>> + LOCATION, which is attempting to match on WORD. */
>> +
>> +static VEC (char_ptr) *
>> +collect_explicit_location_matches (struct event_location *location,
>> + enum explicit_location_match_type what,
>> + const char *word)
>> +{
[snip]
>> + case MATCH_LABEL:
>> + /* Not supported. */
>> + break;
>> +
>> + default:
>> + gdb_assert_not_reached (_("unhandled explicit_location_match_type"));
>
> ====
> No need for _() here.
Fixed.
>
>> + }
>> +
>> + return matches;
>> +}
>> +
>> +/* A convenience macro to (safely) back up P to the previous
>> + word. */
>
> ====
> This comment can be just one line.
> I'd rather a comment run past the soft limit (and still fit within the
> hard limit) than use an extra line just for one word.
>
Fixed.
>> +
>> +static const char *
>> +backup_text_ptr (const char *p, const char *text)
>> +{
>> + while (p > text && isspace (*p))
>> + --p;
>> + for (; p > text && !isspace (p[-1]); --p)
>> + ;
>> +
>> + return p;
>> +}
>> +
>
> ====
> Missing function comment.
>
Doh! Added.
>> +static VEC (char_ptr) *
>> +explicit_location_completer (struct cmd_list_element *ignore,
>> + struct event_location *location,
>> + const char *text, const char *word)
>> +{
>> + const char *p;
>> + VEC (char_ptr) *matches = NULL;
>> +
>> + /* Find the beginning of the word. This is necessary because
>> + we need to know if we are completing an option name or value. We
>> + don't get the leading '-' from the completer. */
>> + p = backup_text_ptr (word, text);
>> +
>> + if (*p == '-')
>> + {
>> + size_t len = strlen (word);
>> +
>> + /* Completing on option name. */
>
> ====
> I couldn't get "b -<tab><tab>" to work. Can do?
>
Unfortunately, "-" is a valid linespec, so no, "b -<TAB><TAB>" will not
do anything.
>> +
>> + /* Skip over the '-'. */
>> + ++p;
>> +
>> + if (strncmp (p, "source", len) == 0)
[snip]
>> + /* Now gather matches */
>> + matches = collect_explicit_location_matches (location, what, new_word);
>
> ====
> Something's not right here as "b -f <tab>" segv's because new_word is NULL.
>
Yes, indeed. The struct explicit_location uses NULL for unspecified
parameters, and these consequently get passed as new_word to
collect_explicit_location_matches, which passes it to GDB's completion
functions, which do NOT like NULL.
I've fixed this and added tests for them. Thank you for catching this
serious bug.
Keith