From patchwork Wed Sep 3 04:02:32 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Doug Evans X-Patchwork-Id: 2635 Received: (qmail 15974 invoked by alias); 3 Sep 2014 04:03:20 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 15953 invoked by uid 89); 3 Sep 2014 04:03:17 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL, BAYES_00, FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-pd0-f175.google.com Received: from mail-pd0-f175.google.com (HELO mail-pd0-f175.google.com) (209.85.192.175) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Wed, 03 Sep 2014 04:03:15 +0000 Received: by mail-pd0-f175.google.com with SMTP id ft15so10083622pdb.6 for ; Tue, 02 Sep 2014 21:03:13 -0700 (PDT) X-Received: by 10.70.130.138 with SMTP id oe10mr2192349pdb.115.1409716993539; Tue, 02 Sep 2014 21:03:13 -0700 (PDT) Received: from seba.sebabeach.org.gmail.com (173-13-178-50-sfba.hfc.comcastbusiness.net. [173.13.178.50]) by mx.google.com with ESMTPSA id fh10sm7525398pdb.71.2014.09.02.21.03.12 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 02 Sep 2014 21:03:12 -0700 (PDT) From: Doug Evans To: Sergio Durigan Junior Cc: Jan Kratochvil , gdb-patches@sourceware.org Subject: Re: [PATCH] PR python/16699: GDB Python command completion with overriden complete vs. completer class References: <87a9btqhe3.fsf@fleche.redhat.com> <87y4xwqn71.fsf@fleche.redhat.com> <20140708153221.GA15767@host2.jankratochvil.net> <87iolow0ad.fsf@redhat.com> <87a96ikmqf.fsf@redhat.com> Date: Tue, 02 Sep 2014 21:02:32 -0700 In-Reply-To: <87a96ikmqf.fsf@redhat.com> (Sergio Durigan Junior's message of "Tue, 02 Sep 2014 12:31:36 -0400") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 X-IsSubscribed: yes Sergio Durigan Junior 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 >> >> 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) >> : 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 >> >> 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? I have no more comments so LGTM with those changes. --- 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