On 11/01/2008, Jim Blandy <jimb@codesourcery.com> wrote:
>
> "Rob Quill" <rob.quill at gmail.com> writes:
> > Please find attached the revised and much simpler patch. One of the
> > drawbacks of the new patch is that you cannot check the scope of
> > structure members, which is quite inconvenient, but I can't really fix
> > that without resorting to the old style of patch.
> >
> > I think that for the moment this patch is sufficient and definitely
> > provides and improvement, but I think in future it would be good to be
> > able to check arbitrary expressions, but as you say in your previous
> > email, this requires some significant reworking of the patch. It also
> > depends on how much people will find it useful. Michael, how do you
> > feel about this? (As you seemed interested in the previous attempt) If
> > there is enough interest then I am more than happy to spend some time
> > reworking the old patch at a later date.
>
> Unfortunately, I can't try out this patch.  Absurdly, the default
> output format for 'diff' can't be used reliably by patch, because it
> doesn't include any context lines (unchanged text surrounding the
> changed text), which 'patch' uses to ensure that the change doesn't
> conflict with other changes made to the code.
>
> Use 'cvs diff -u' or 'cvs diff -c' to produce unified or context
> diffs.  Unified diffs seem to be the more widely preferred form these
> days.  All patches posted to this list (or pretty much to any other
> open source list) should be in unified or context diff form.
>
> I have the following in my ~/.cvsrc file, so I don't have to remember
> to type '-u' all the time:
>
>         diff -u
>

Sorry about that, the attached patch is done with cvs diff -up

> >>          parse_number ("1", 1, 0, &val);
> >>          write_exp_elt_opcode (OP_LONG);
> >>          write_exp_elt_type (val.typed_val_int.type);
> >>          write_exp_elt_longcst ((LONGEST)val.typed_val_int.val);
> >>          write_exp_elt_opcode (OP_LONG);
>
> You've written out this code three times; that's quite a chunk.  I'd
> recommend first deciding whether the symbol is in scope (checking
> '$3.sym', and then calling lookup_minimal_symbol, as you do now), and
> computing a one or a zero from that.
>
> Then, did you know that you can get away with, simply:
>
>           struct type *int_type = builtin_type (current_gdbarch)->builtin_int;
>           write_exp_elt_opcode (OP_LONG);
>           write_exp_elt_type (int_type);
>           write_exp_elt_longcst (<flag value>);
>           write_exp_elt_opcode (OP_LONG);
>
> So there's no need to parse the number from a string.
>

Thanks for the tip :) I think the only example I had seen of parsing
actual values was where it used the parse number function, so I
assumed that was the right thing to use.

> > 1313a1357,1361
> >> static const struct token tokentab9[] =
> >>   {
> >>     {"$in_scope", IN_SCOPE, BINOP_END}
> >>   };
> >>
> > 1375a1424,1432
> >>   /* Code for recognising the $in_scope token. */
> >>   /* See if it is a special token of length 9.  */
> >>   for (i = 0; i < sizeof tokentab9 / sizeof tokentab9[0]; i++)
> >>     if (strncmp (tokstart, tokentab9[i].operator, 9) == 0)
> >>     {
> >>          lexptr += 9;
> >>          yylval.opcode = tokentab9[i].opcode;
> >>          return tokentab9[i].token;
> >>     }
>
> I think I would rather see $in_scope recognized like the other
> keywords (see "Catch specific keywords.") than at this point as a
> symbolic token.

My mistake, I didn't realise before that keywords were declared here.

Rob