From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13397 invoked by alias); 12 Mar 2013 20:43:29 -0000 Received: (qmail 13388 invoked by uid 22791); 12 Mar 2013 20:43:28 -0000 X-SWARE-Spam-Status: No, hits=-7.7 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_SPAMHAUS_DROP,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,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; Tue, 12 Mar 2013 20:43:23 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r2CKhN4k001566 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 12 Mar 2013 16:43:23 -0400 Received: from barimba (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r2CKhLmn027509 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Tue, 12 Mar 2013 16:43:22 -0400 From: Tom Tromey To: Phil Muldoon Cc: "gdb-patches\@sourceware.org" Subject: Re: [patch][python] 2 of 5 - Frame filter MI code changes. References: <513E5707.8080404@redhat.com> Date: Tue, 12 Mar 2013 20:43:00 -0000 In-Reply-To: <513E5707.8080404@redhat.com> (Phil Muldoon's message of "Mon, 11 Mar 2013 22:13:27 +0000") Message-ID: <87vc8wcpsm.fsf@fleche.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (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: 2013-03/txt/msg00553.txt.bz2 >>>>> "Phil" == Phil Muldoon writes: Phil> + Phil> +static int Phil> +parse_no_frames_option (char *arg) Needs an intro comment. 'arg' should be const. Phil> + if (arg && (strcmp (arg, "--no-frame-filters") == 0)) We recently decided to use explicit NULL checks, like 'arg != NULL'. Phil> + /* Parse arguments. In this instance we are just looking for Phil> + --no-frame-filters. */ Phil> + while (1) Phil> + { Phil> + int oind = 0; Phil> + char *oarg; Phil> + int opt = mi_getopt ("-stack-list-frames", argc, argv, Phil> + opts, &oind, &oarg); I don't think it is correct to set oind in the loop. It may work now but it will be wrong if we ever add more options. Phil> + if ((argc > 3 && ! raw_arg) || (argc == 1 && ! raw_arg) Phil> + || (argc == 2 && raw_arg)) Phil> + error (_("-stack-list-frames: Usage: [--no-frame-filters] [FRAME_LOW FRAME_HIGH]")); This code should use oind and not check raw_arg. Actually the rest of the function should use oind to index into argv. Phil> + /* We cannot pass -1 to frame_low, as that would signify a Phil> + relative backtrace from the tail of the stack. So, in the case Phil> + of frame_low == -1, assign and increment it. */ Phil> + if (py_frame_low == -1) Phil> + py_frame_low++; I didn't check, but I assume it is ok for frame_high==-1 in the call? Phil> + /* Run the inbuilt backtrace if there are no filters registered, or Phil> + if there was an error in the Python backtracing output, or if Phil> + frame-filters are disabled. */ Phil> + if (! frame_filters || raw_arg || result == PY_BT_ERROR Phil> + || result == PY_BT_NO_FILTERS) It seems to me that the PY_BT_ERROR case should not be here, since the MI client could get a partially filtered trace followed by the normal trace. 'result' is initialized to 0 but it seems like it should be initialized to some PY_BT_ enum value; and have the right type. Phil> + error (_("-stack-list-locals: Usage: [--no-frame-filters] PRINT_VALUES")); I think -stack-list-frames takes "-no-frame-filters" but this and others take the "--" form. That seems confusing. Phil> + result = apply_frame_filter (frame, flags, print_value, Phil> + current_uiout, 0,0); Missing space after the ",". Phil> + if (! frame_filters || raw_arg || result == PY_BT_ERROR Phil> + || result == PY_BT_NO_FILTERS) Phil> + { Phil> + list_args_or_locals (locals, print_value, frame); Same comments about PY_BT_ERROR and 'result'. Actually this applies everywhere. Tom