From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22132 invoked by alias); 10 Jan 2008 01:00:26 -0000 Received: (qmail 22121 invoked by uid 22791); 10 Jan 2008 01:00:25 -0000 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.31) with ESMTP; Thu, 10 Jan 2008 00:59:56 +0000 Received: (qmail 17280 invoked from network); 10 Jan 2008 00:59:54 -0000 Received: from unknown (HELO localhost) (jimb@127.0.0.2) by mail.codesourcery.com with ESMTPA; 10 Jan 2008 00:59:54 -0000 To: "Rob Quill" Cc: gdb-patches@sourceware.org Subject: Re: New scope checking patch References: From: Jim Blandy Date: Thu, 10 Jan 2008 01:00:00 -0000 In-Reply-To: (Rob Quill's message of "Mon, 12 Nov 2007 16:29:14 +0000") Message-ID: User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.0.50 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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: 2008-01/txt/msg00222.txt.bz2 "Rob Quill" writes: > The attached patch adds the ability to evaluate whether expressions > are in scope without giving an error message. For example, if you were > using a command file to control GDB then it it may not be ideal that > the script stop executing due to a variable not being in scope at > given position. Hi, Rob. I'm sorry for dropping the review here. Some mechanical points: - Patches should have ChangeLog entries. For example, take a look at this post: http://sourceware.org/ml/gdb-patches/2008-01/msg00144.html - Use 'cvs diff' to get the difference between your current sources and the public file revisions they're based on; patches that affect CVS metadata like "CVS/Entries" files can be treacherous if applied unawares. :) - GDB's code ought to follow the GNU Coding Standards; in particular, lines shouldn't be longer than eighty characters. (c-exp.y in particular has a lot of particularly horrible formatting, but we've got to fight back; new patches should not make things worse.) Also, indentation should follow GNU style, for consistency with the rest of the code. - The patch includes a number of hunks that are strictly whitespace changes in code that's not otherwise involved in the change; they require reviewers to check the 'before' and 'after' text by hand, to see whether anything has actually changed. Try to keep extraneous hunks out of patches. As to the patch: The functionality I originally suggested was to have an operator $in_scope(X), where X is an identifier. What you've implemented is a change that allows X to be an arbitrary expression, such that $in_scope (X) is true if all the identifiers in X are bound. I gather this was more useful for your original purpose, scripting GDB. That's fine, but it does make the change a bit harder. Having the $in_scope operator set a global flag which is cleared by evaluate_expression isn't the ideal approach; global flags often have effects beyond what was intended. For example: - If I enter the expression '$in_scope (x) + y', the patch will evaluate the entire expression using evaluate_for_scope, not just the '$in_scope (x)' part. So $in_scope doesn't act like an operator checking its operands; its presence affects the evaluation of the whole expression. - GDB sometimes parses an expression, and then evaluates it many times over a long period of time (say, conditions for breakpoints). Sometimes it parses an expression, and then doesn't evaluate it for a while (say, 'display' expressions set when the program isn't running). So setting check_scope when the expression is parsed, and then checking and clearing it when the expression is evaluated, won't work --- check_scope will be clear for expressions that need it (in the first case) and set for expressions that don't (the second case). The patch also contains what looks like some abandoned experiments, or changes that belong in evaluate_subexp_for_scope but escaped into other functions. Please be careful with this: @@ -1438,8 +1468,11 @@ evaluate_subexp_standard (struct type *e case BINOP_CONCAT: arg1 = evaluate_subexp_with_coercion (exp, pos, noside); arg2 = evaluate_subexp_with_coercion (exp, pos, noside); - if (noside == EVAL_SKIP) - goto nosideret; + + return value_binop (arg1, arg2, BINOP_LOGICAL_AND); + + if (noside == EVAL_SKIP) + goto nosideret; if (binop_user_defined_p (op, arg1, arg2)) return value_x_binop (arg1, arg2, op, OP_NULL, noside); else > This also affects compound expressions that involve the literal 0. For > example, $in_scope(0+3) will return 0, as an expression with a binary > operator is considering in scope iff both of its operands are in > scope. If you want to have $in_scope take an arbitrary expression as its operand, then cases like this will need to be handled properly. Otherwise, people won't be able to reliably take whatever expressions they have at hand and apply $in_scope to them. If restricting $in_scope (X) to the case where X is a single identifier is useful to you, the patch can be much simpler. Have the grammar rule be: exp: IN_SCOPE '(' name ')' { ... } where the '...' calls lookup_symbol itself, the way the 'variable' non-terminal does, and then produces an OP_LONG whose value is zero or one, depending on whether lookup_symbol found anything? That way, no changes are needed to eval.c at all.