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:
> On Tuesday, August 19 2014, I wrote:
>
>> On Tuesday, July 08 2014, I wrote:
>>
>>> Thank you for the catches.  I will update my local version.
>>
>> Ping.
>
> Ping^2.
>
>> -- 
>> Sergio
>> GPG key ID: 0x65FC5E36
>> Please send encrypted e-mail if possible
>> http://sergiodj.net/
>>
>> gdb/
>> 2014-08-19  Sergio Durigan Junior  <sergiodj@redhat.com>
>>
>> 	PR python/16699
>> 	* cli/cli-decode.c (set_cmd_completer_handle_brkchars): New
>> 	function.
>> 	(add_cmd): Set "completer_handle_brkchars" to NULL.
>> 	* cli/cli-decode.h (struct cmd_list_element)
>> 	<completer_handle_brkchars>: New field.
>> 	* command.h (completer_ftype_void): New typedef.
>> 	(set_cmd_completer_handle_brkchars): New prototype.
>> 	* completer.c (set_gdb_completion_word_break_characters): New
>> 	function.
>> 	(complete_line_internal): Call "completer_handle_brkchars"
>> 	callback from command.
>> 	* completer.h: Include "command.h".
>> 	(set_gdb_completion_word_break_characters): New prototype.
>> 	* python/py-cmd.c (cmdpy_completer_helper): New function.
>> 	(cmdpy_completer_handle_brkchars): New function.
>> 	(cmdpy_completer): Adjust to use cmdpy_completer_helper.
>> 	(cmdpy_init): Set completer_handle_brkchars to
>> 	cmdpy_completer_handle_brkchars.
>>
>> gdb/testsuite/
>> 2014-08-19  Sergio Durigan Junior  <sergiodj@redhat.com>
>>
>> 	PR python/16699
>> 	* gdb.python/py-completion.exp: New file.
>> 	* gdb.python/py-completion.py: Likewise.

Hi.

I've spent some more time looking at the patch.

I have one style nit and one request.

Style nit: I see lots of places that do "if (!ptr)".
Convention is to write this as "if (ptr == NULL)".

Request:
The "why" of things is explained sufficiently well in your
original email:
https://sourceware.org/ml/gdb-patches/2014-03/msg00301.html
And while I can appreciate this text being added to the commit message,
I don't want to have to read commit logs to have a basic understanding
of the "why" of the code. [I'm all for more deeper understanding
being deferred to commit logs as appropriate, but I should be able
to get a basic understanding from the code itself.]

Can you add something descriptive to the code?

For example, how about something like this?

--- python/py-cmd.c=    2014-09-02 20:28:57.838267408 -0700
+++ python/py-cmd.c     2014-09-02 20:59:40.026083464 -0700
@@ -219,6 +219,14 @@ cmdpy_function (struct cmd_list_element
    returns that variable, without actually calling the Python method
    again.  This saves us one Python method call.

+   The reason for this two step dance is that we need to know the set
+   of "brkchars" to use early on, before we actually try to perform the
+   completion.  But if a Python command supplies a "complete" method then
+   we have to call that method first: it may return as its result the kind
+   of completion to perform and that will in turn specify which brkchars
+   to use.  IOW, we need the result of the "complete" method before we
+   actually perform the completion.
+
    It is important to mention that this function is built on the
    assumption that the calls to cmdpy_completer_handle_brkchars and
    cmdpy_completer will be subsequent with nothing intervening.  This


I have no more comments so LGTM with those changes.