Re: New scope checking patch

Rob Quill
2008-01-17T19:32:52Z

[-- Attachment #1: Type: text/plain, Size: 3573 bytes --]

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: in_scope.patch --]
[-- Type: text/x-diff; name=in_scope.patch, Size: 1544 bytes --]

Index: gdb/c-exp.y
===================================================================
RCS file: /cvs/src/src/gdb/c-exp.y,v
retrieving revision 1.42
diff -u -r1.42 c-exp.y
--- gdb/c-exp.y 9 Jan 2008 19:27:15 -0000 1.42
+++ gdb/c-exp.y 13 Jan 2008 20:22:26 -0000
@@ -208,6 +208,8 @@
%token TRUEKEYWORD
%token FALSEKEYWORD

+/* $in_scope opperator */
+%left IN_SCOPE

%left ','
%left ABOVE_COMMA
@@ -251,6 +253,32 @@
;

/* Expressions, not including the comma operator. */
+exp : IN_SCOPE '(' name_not_typename ')'
+ {
+ struct type *int_type;
+
+ /* If there are no symbols then just stop right away */
+ if (!have_full_symbols () && !have_partial_symbols ())
+ error ("No symbol table is loaded. Use the \"file\" command.");
+
+ /* Otherwise, prepare to write out the value */
+ int_type = builtin_type (current_gdbarch)->builtin_int;
+ write_exp_elt_opcode (OP_LONG);
+ write_exp_elt_type (int_type);
+
+ if ($3.sym || lookup_minimal_symbol(copy_name($3.stoken), NULL, NULL))
+ {
+ write_exp_elt_longcst ((LONGEST) 1);
+ }
+ else
+ {
+ write_exp_elt_longcst ((LONGEST) 0);
+ }
+
+ write_exp_elt_opcode (OP_LONG);
+ }
+ ;
+
exp : '*' exp %prec UNARY
{ write_exp_elt_opcode (UNOP_IND); }
;
@@ -1678,6 +1706,9 @@
/* Catch specific keywords. Should be done with a data structure. */
switch (namelen)
{
+ case 9:
+ if (strncmp (tokstart, "$in_scope", 9) == 0)
+ return IN_SCOPE;
case 8:
if (strncmp (tokstart, "unsigned", 8) == 0)
return UNSIGNED;