Re: [PATCH] PR python/16699: GDB Python command completion with overriden complete vs. completer class

[Doug Evans <xdje42@gmail.com>]

Sergio Durigan Junior <sergiodj@redhat.com> writes:
> [...]
> Thanks.  WDYT of the following patch?

Hi.

fwiw it's too bad the ability to plug in different completers isn't more,
I dunno, parameterized (couldn't pick a better term, apologies -
I thought of "object oriented" but that carries its own baggage).
Performing completion obviously involves specifying more than a just
single function (witness the comparison of the completer with specific
functions).

Plus it's more than specifying brkchars.
Witness code like this:

		      /* Many commands which want to complete on
			 file names accept several file names, as
			 in "run foo bar >>baz".  So we don't want
			 to complete the entire text after the
			 command, just the last word.  To this
			 end, we need to find the beginning of the
			 file name by starting at `word' and going
			 backwards.  */
		      for (p = word;
			   p > tmp_command
			     && strchr (gdb_completer_file_name_break_characters, p[-1]) == NULL;
			   p--)
			;

IWBN if a "completer" object described how to do all these three things.
Then the special case code for filename_completer (and location_completer)
in completer.c could disappear.  But maybe that's a patch for another day.


Regarding the hack of using a static local to pass data from
handle_brkchars to handle_completion, I know it's a hacky pragmatic
choice.  To get the reference counting right the code assumes that
if the handle_brkchars phase is done then the handle_completion
phase will be done too, right?  I wonder if a SIGINT could sneak in
there between the two passes (either today or tomorrow).
Maybe the code in cmdpy_completer_helper for handle_brkchars_p could
first check whether resultobj is already non-NULL, and decrement its
reference count before setting it to NULL?  And cmdpy_completer_helper
could be defined to return a borrowed reference to resultobj?
Dunno, just thinking out loud.
Something puzzles me though: If it's ok to cache the completion result from the
handle_brkchars pass to the handle_completion pass, why have two passes?
It feels like there must be a case where this caching of the result
in a static local from one pass to the next won't work.


Another question:
I noticed complete_command doesn't do this two-phase dance
of handle_brkchars followed by handle_completions.  Should it?
It just goes straight to handle_completions.
[Maybe that explains the difference from using TAB.  Dunno off hand.]
It seems like complete_command is trying to hand-code its own
handle_brkchars handling:

static void
complete_command (char *arg, int from_tty)
{
  int argpoint;
  char *point, *arg_prefix;
  VEC (char_ptr) *completions;

  dont_repeat ();

  if (arg == NULL)
    arg = "";
  argpoint = strlen (arg);

  /* complete_line assumes that its first argument is somewhere
     within, and except for filenames at the beginning of, the word to
     be completed.  The following crude imitation of readline's
     word-breaking tries to accomodate this.  */
  point = arg + argpoint;
  while (point > arg)
    {
      if (strchr (rl_completer_word_break_characters, point[-1]) != 0)
        break;
      point--;
    }

  arg_prefix = alloca (point - arg + 1);
  memcpy (arg_prefix, arg, point - arg);
  arg_prefix[point - arg] = 0;

  completions = complete_line (point, arg, argpoint);

  ...
}

TAB and the complete command should work identically of course,
but for your testcase, maybe you should test both just to verify
both work reasonably well (even if not identically).
Given that complete_command doesn't do the two phase dance,
does it work with your patch?
Maybe it does, but IWBN to confirm that.