Re: New scope checking patch

[Jim Blandy <jimb at codesourcery dot com>]

"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): ...