From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6545 invoked by alias); 24 Jul 2012 20:39:24 -0000 Received: (qmail 6534 invoked by uid 22791); 24 Jul 2012 20:39:23 -0000 X-SWARE-Spam-Status: No, hits=-6.5 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 24 Jul 2012 20:38:57 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q6OKcrR4012842 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 24 Jul 2012 16:38:54 -0400 Received: from barimba (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q6OKcqtU001064 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Tue, 24 Jul 2012 16:38:52 -0400 From: Tom Tromey To: Yao Qi Cc: Subject: Re: [PATCH 1/6] new observer command_option_changed. References: <1343146252-22558-1-git-send-email-yao@codesourcery.com> <1343146252-22558-2-git-send-email-yao@codesourcery.com> Date: Tue, 24 Jul 2012 20:39:00 -0000 In-Reply-To: <1343146252-22558-2-git-send-email-yao@codesourcery.com> (Yao Qi's message of "Wed, 25 Jul 2012 00:10:47 +0800") Message-ID: <87394gopwj.fsf@fleche.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-07/txt/msg00504.txt.bz2 >>>>> "Yao" == Yao Qi writes: Yao> This patch is to add an extra field 'notify_observer_p' to identify Yao> the command will notify observer 'command_option_changed', and teach Yao> GDB to check the change of command option, and notify observer. I read through the series to find out why this was needed. My understanding is that the idea is that options will be segregated into two kinds: those which report via the observer, and others; and that the decision is made in the gdb sources. If that is accurate, I wonder if you could explain the rationale for it. Another possible design would be to report all option changes; or to let the MI client choose. The reason I ask is that making the decision statically seems tricky. When adding an option, what criteria do we use to decide? How do we know what MI users want? Or what if we want to add a Python hook here -- maybe we would switch many more options over; but does switching an option from non-reporting to reporting cause issues for existing MI clients? (I assume there is at least a bandwidth concern...) Yao> +const char *auto_boolean_enums[] = { "on", "off", "auto", NULL }; It seems like now this should be 'const char * const'. I realize you're just moving it, so normally it would be exempt from this kind of tweakery, but in this case it is also now exported... Yao> + if (c->notify_observer_p) Yao> + { Yao> + char *s = (char *) auto_boolean_enums[val]; I don't think the cast is needed. Yao> +@deftypefun void command_option_changed (const char *@var{command}, const char *@var{option}) Yao> +The option of some @code{set} commands in console are changed. This method is called after Yao> +a command @code{set command option}. Yao> +@end deftypefun The lines in the body are all too long. I think it would be good to describe the meaning of the arguments. What happens in the case where an option has a validation function that fails? IIRC gdb has an internal design flaw here. Tom