Hi Doug,

Thanks again for reviewing my patches!

On 07/26/2015 11:29 AM, Doug Evans wrote:
>> 	* ada-lang.c: Include cp-support.h
>
> ada-foo including cp-foo?
> We need to clean this up.
> Things are even muddier as this stuff is recorded in
> block.language_specific.cplus_specific, so cleaning this up now will be good.
>
> I gather cp-support.h is needed here for struct using_direct.
> I think (at least) three things need to happen.
> 1) Move the definition of struct using_direct out of cp-support.h.
> 2) Move the definition of cp_add_using_directive out of cp-namespace.c.
>     And delete the leading "cp_".
> 3) Reorg block.language_specific.
>
> For the first two I'd be ok with lang-support.[ch].
> Yeah, it'll be a small file for now, but that's ok IMO.
> Alternatives are language.[ch] or block.[ch].

Quoting myself on IRC:
> xdje: hello! in
> https://sourceware.org/ml/gdb-patches/2015-07/msg00768.html you
> suggested to move namespace bits in a new file
>
> I think that makes sense too, but lang-support.* does not
>
> I mean… symtab, block, etc. all are about source debugging… and thus
> languages support
>
> so I’m tempted to create instead namespace.* or else to move this to
> block.*
>
> what do you think?

So I created namespace.[ch]. Please tell me if that's fine, otherwise I 
can switch to the names you suggested.

> For (3), in the absence of needing anything more at the moment,
> I'd be ok with replacing:
>
>    union
>    {
>      struct
>      {
>        /* Contains information about namespace-related info relevant to
> 	 this block: using directives and the current namespace
> 	 scope.  */
>
>        struct block_namespace_info *the_namespace;
>      }
>      cplus_specific;
>    }
>    language_specific;
>
> with just:
>
>    /* Contains information about namespace-related info relevant to
>       this block: using directives and the current namespace scope.  */
>    struct block_namespace_info *the_namespace;
>
> though I'd probably rename "the_namespace" to "namespace_info"
> or some such.

Done.

> Missing "All callers updated."
>
>> 	(cp_scan_for_anonymous_namespaces): Update call to
>> 	cp_add_using_directive.
>
> ====
> Don't write individual changelog entries for updates to function calls.
> Instead, just write "All callers updated." at the description of
> the function that was changed.

Understood. I updated the ChangeLog accordingly.

>> @@ -1018,6 +1025,7 @@ prepare_for_building (const char *name, CORE_ADDR start_addr)
>>        a symtab, or by the really_free_pendings cleanup.  */
>>     gdb_assert (file_symbols == NULL);
>>     gdb_assert (global_symbols == NULL);
>> +  gdb_assert (global_using_directives == NULL);
>
> ====
> I've put a lot of time into cleaning up buildsym.[ch] but we
> still have a long way to go (for at least a few reasons).
> I don't have a strong preference, but prepare_for_building should do
> *something* with local_using_directives.  I think it doesn't because
> I was making so many changes I punted on this part.  What's there
> now leaves one wondering (e.g., why does buildsym_init initialize
> local_using_directives? and why doesn't reset_symtab_globals reset
> local_using_directives? and there are more.).
> But I'm happy with leaving this for later,
> I just wanted to point out the issue.

Understood, thank you for the background. I tried anyway to make this as 
consistent as possible between global/local_using_directives: I added 
resetting local_using_directives in prepare_for_building and 
reset_symtab_globals and I removed the reset in buildsym_init.

>> +	  cp_add_using_directive ((context_stack_depth > 0)
>> +				    ? &local_using_directives
>> +				    : &global_using_directives,
>
> ====
> Indentation is wrong.
> Typically what we do is surround the entire argument in () and indent
> like so:
>
>     (foo > 0
>      ? bar
>      : baz),

Thanks. I have fixed this occurence and the other ones.

> Plus this needs a language_ada check (right?).

Yes indeed! As the check has a big comment explaining it, I've created a 
function (using_directives) to have a single place for it and updated 
code to reference it.

So the updated patch is attached, tested with a trunk GCC on 
x86_64-linux. For the record, the corresponding GCC patch isn't in trunk 
yet (pending review).

-- 
Pierre-Marie de Rodat