Re: New scope checking patch

Jim Blandy
2008-01-18T18:48:34Z

"Rob Quill" <rob.quill at gmail.com> writes:
>> > + if (!have_full_symbols () && !have_partial_symbols ())
>> > + error ("No symbol table is loaded. Use the \"file\" command.");
>>
>> Too much indentation?
>
> Well, you would think so, but I've changed the tab spacing in vim to 8
> and the way I have formatted it matches how everything else is
> formatted.

I swear I'm not harrassing you. :) Once we can get these minor things
taken care of, the patch looks ready to me.

The error should be indented two columns within the 'if'. Here's what
I see in most every other use of 'error' in the grammar section of the
file.

if (new_type == NULL)
error ("No type \"%s\" within class or namespace \"%s\".",
ncopy, TYPE_NAME (type));

>> Closing brace should get its own line, since the opening brace did.
>
> This is what I thought too, but if you look at the rest of the file
> (for example the case for "exp : exp ARROW name") the open brace
> always starts on a new line and always ends on the same line.

Right you are.

>> When I view this (again, with 8-column tab stops), the 'case' is not
>> lined up with the other cases, and the 'return' is not indented two
>> spaces within the 'if'.
>
> Sorry, missed that. I've attached the fixed copy. I believe that the
> amount of indentation matches the layout of the current file, and that
> even though it looks like a lot in the context of the patch, all the
> other grammar rules in the file use the same large amount of
> indentation.

This code block looks fine to me in your latest patch.

The only other point left is the ChangeLog entry. The one you posted
was:

2008-01-17 Rob Quill <rob.quill@gmail.com>
* c-exp.y : Add $in_scope as a type of expression.

This needs a little rearrangement, and a bit more detail. A model to
follow might be:

2008-01-07 Vladimir Prus <vladimir@codesourcery.com>

Ignore change in name of dynamic linker during
execution on Solaris. This also unbreaks pending breakpoints.

* solist.h (struct target_so_ops): New field same.
* solib-svr4.c (svr4_same): New.
(_initialize_svr4_solib): Register svr4_same.
* solib.c (update_solib_list): Use ops->same, if available.

If the change merits it, it's nice to have a (brief) summary sentence
at the top (substantial explanation belongs in comments in the code).
Your "Add $in_scope ..." might go there. Then, each change is
attributed to a particular function, top-level declaration, or
whatever grouping makes sense (in documentation, we put the section
name in the parens).

So your ChangeLog entry would need to mention IN_SCOPE as a new token
you're defining, 'exp' (I guess) as the non-terminal symbol to which
you're adding a grammar rule, and 'yylex' as the function to which
you're adding a bit of code.

2008-01-17 Rob Quill <rob.quill@gmail.com>

Add $in_scope as a type of expression.
* c-exp.y (IN_SCOPE): ...
(exp): ...
(yylex): ...