Re: New scope checking patch

[Jim Blandy <jimb at codesourcery dot com>]

"Rob Quill" <rob.quill at gmail.com> 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.