From: Phil Muldoon <pmuldoon@redhat.com>
To: Tom Tromey <tromey@redhat.com>
Cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
Subject: Re: [patch][python] 2 of 5 - Frame filter MI code changes.
Date: Fri, 07 Dec 2012 13:56:00 -0000 [thread overview]
Message-ID: <50C1F56E.5060506@redhat.com> (raw)
In-Reply-To: <87ip8gtoex.fsf@fleche.redhat.com>
On 12/05/2012 05:11 PM, Tom Tromey wrote:
>>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> 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
next prev parent reply other threads:[~2012-12-07 13:56 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-11-30 14:31 Phil Muldoon
2012-12-05 17:11 ` Tom Tromey
2012-12-07 13:56 ` Phil Muldoon [this message]
2012-12-10 21:03 ` Tom Tromey
2013-02-05 12:08 ` Phil Muldoon
2013-02-07 21:32 ` Tom Tromey
2013-02-20 15:17 ` Phil Muldoon
2013-02-20 20:37 ` Tom Tromey
2013-03-11 22:13 Phil Muldoon
2013-03-12 20:43 ` Tom Tromey
2013-03-12 20:52 ` Phil Muldoon
2013-03-13 12:15 ` Phil Muldoon
2013-03-13 17:48 ` Tom Tromey
2013-03-13 19:41 ` Phil Muldoon
2013-03-13 20:27 ` Tom Tromey
2013-03-13 20:53 ` Phil Muldoon
2013-03-13 20:56 ` Tom Tromey
2013-03-13 21:10 ` Phil Muldoon
2013-03-14 17:54 ` Tom Tromey
2013-03-14 19:35 ` Phil Muldoon
2013-04-22 16:01 Phil Muldoon
2013-04-26 11:19 ` Tom Tromey
2013-05-06 8:23 Phil Muldoon
2013-05-06 20:42 ` Tom Tromey
2013-05-07 8:23 ` Phil Muldoon
2013-05-07 14:02 ` Tom Tromey
2013-05-08 10:18 ` Phil Muldoon
2013-05-08 19:47 ` Tom Tromey
2013-05-10 10:45 ` Phil Muldoon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=50C1F56E.5060506@redhat.com \
--to=pmuldoon@redhat.com \
--cc=gdb-patches@sourceware.org \
--cc=tromey@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox