From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32546 invoked by alias); 7 Dec 2012 13:56:12 -0000 Received: (qmail 32538 invoked by uid 22791); 7 Dec 2012 13:56:11 -0000 X-SWARE-Spam-Status: No, hits=-6.9 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS 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; Fri, 07 Dec 2012 13:56:01 +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 qB7Du0VH027332 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 7 Dec 2012 08:56:00 -0500 Received: from localhost.localdomain (ovpn-116-39.ams2.redhat.com [10.36.116.39]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id qB7DtwU4030024; Fri, 7 Dec 2012 08:55:59 -0500 Message-ID: <50C1F56E.5060506@redhat.com> Date: Fri, 07 Dec 2012 13:56:00 -0000 From: Phil Muldoon MIME-Version: 1.0 To: Tom Tromey CC: "gdb-patches@sourceware.org" Subject: Re: [patch][python] 2 of 5 - Frame filter MI code changes. References: <50B8C333.4070008@redhat.com> <87ip8gtoex.fsf@fleche.redhat.com> In-Reply-To: <87ip8gtoex.fsf@fleche.redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes 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-12/txt/msg00167.txt.bz2 On 12/05/2012 05:11 PM, Tom Tromey wrote: >>>>>> "Phil" == Phil Muldoon writes: > Phil> +/* True if we want to allow Python-based frame filters. */ > Phil> +static int frame_filters = 0; > Phil> + > Phil> +void > Phil> +stack_enable_frame_filters (void) > Phil> +{ > Phil> + frame_filters = 1; > Phil> +} > > I don't think you need this function, see below. > > Phil> +static int > Phil> +parse_no_frames_option (char *arg) > Phil> +{ > Phil> + if (arg && (strcmp (arg, "--no-frame-filters") == 0)) > Phil> + return 1; > Phil> + > Phil> + return 0; > > I'd prefer it if the various callers were changed to use mi_getopt. > This provides uniformity and lets us add options later. If there was uniformity then I would agree, but as far as I looked there wasn't. Some MI commands use mi_getopt, some parse their own options, some allow long options ("--"), others do not, and mi_getopt does not handle long options in any case (and huge amounts of other useful getopt functions too). I wrote a patch for mi_getopts to handle long options, but why do we even need another implementation of getopt like functionality? So I decided to just leave be, and parse options as each command has previously done so. Maybe I should have written a cleanup patch before hand. I wanted to mention something else about MI. I recently discovered in the GDB manual that -stack-list-locals, -stack-list-arguments are considered depreciated. Not even sure if we should add frame filter logic to them. What do you think? > Phil> + if (! raw_arg && frame_filters) > Phil> + { > Phil> + int count = frame_high; > Phil> + int flags = PRINT_LEVEL | PRINT_FRAME_INFO; > Phil> + > Phil> + if (frame_high != -1) > Phil> + count = (frame_high - frame_low) + 1; > Phil> + > Phil> + result = apply_frame_filter (fi, flags, 0, NULL, current_uiout, > Phil> + count); > > I don't think I follow the high/low logic here. > > How does this code strip off the first 'frame_low' frames? fi is unwound to the position of frame_low in a loop preceding this call. This is existing code, and not in the patch context. It is as follows: /* Let's position fi on the frame at which to start the display. Could be the innermost frame if the whole stack needs displaying, or if frame_low is 0. */ for (i = 0, fi = get_current_frame (); fi && i < frame_low; i++, fi = get_prev_frame (fi)); > > Also, Do frame_low and frame_high refer to "raw" or "cooked" frames? > I tend to think they should refer to cooked ones, but I think at least > the answer should be explicit and documented. In the existing mi sense, they just refer to frames on the stack. I followed this logic, but something I am still unsure of is if a frame is elided between frame low, and frame high, if that should be counted. I think it should. > Phil> void > Phil> +mi_cmd_enable_frame_filters (char *command, char **argv, int argc) > Phil> +{ > Phil> + if (argc != 0) > Phil> + error (_("-enable-frame-filters: no arguments allowed")); > Phil> + > Phil> + stack_enable_frame_filters (); > > I think just put this into mi-cmd-stack.c and remove > stack_enable_frame_filters. I was curious about this, I just followed how pretty printing is done. I have no objection though. Cheers, Phil