This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 1/2] namespace support
- From: Tom Tromey <tromey at redhat dot com>
- To: Sami Wagiaalla <swagiaal at redhat dot com>
- Cc: GDB Patches <gdb-patches at sourceware dot org>
- Date: Thu, 04 Jun 2009 18:26:48 -0600
- Subject: Re: [PATCH 1/2] namespace support
- References: <4A16BAB5.80602@redhat.com>
- Reply-to: tromey at redhat dot com
>>>>> "Sami" == Sami Wagiaalla <swagiaal@redhat.com> writes:
Sami> Currently imports statements are stored in the nearest static
Sami> block. Also, these import statements only correspond to the
Sami> implicit importing of annonymous namespaces.
This looks pretty reasonable to me.
A few nits and such...
Sami> +/* This returns the using directives list associated to BLOCK, if
Sami> + any. Each BLOCK_NAMESPACE()->USING already contains all the namespaces
Sami> + imported at that code point - even those from its parent blocks. */
I think the caps on "USING" here are weird.
Maybe just:
The result contains all the [...]
Sami> +/* using directives local to lexical context */
Comments should in general be sentences -- starting with a capital
letter and ending with a period and two spaces.
Here I would probably write:
/* "using" directives local to lexical context. */
... avoiding the capital since "using" is a keyword.
Sami> +EXTERN struct using_direct *using_directives;
Eww, buildsym is full of globals. Not your problem :-), but yet
another thing that would be nice to clean up.
Sami> + /* Pending using directives at the time we entered */
Likewise about the period and spaces.
Sami> - using_list = cp_add_using (name, inner_length, outer_length,
Sami> - using_list);
Sami> + using_directives = cp_add_using (name, inner_length, outer_length,
Sami> + using_directives);
Looks like a formatting change; the old code was indented correctly.
Sami> +static void
Sami> +read_import_statement (struct die_info *die, struct dwarf2_cu *cu)
[...]
Sami> + if (imported_name == NULL)
Sami> + {
Sami> + /* C++ imports from std:: DW_TAG_base_type with no DW_AT_name - why? */
It would be nice to understand this before the patch goes in.
Sami> + /* FIXME: dwarf2_name (die); for the local name after import. */
It would also be nice if we could not add new FIXMEs.
Or is that handled in a later patch?
Sami> + using_directives = cp_add_using (imported_name, strlen (imported_name), 0,
Sami> + using_directives);
Sami> +}
I didn't look -- but are there other buildsym globals which are
actively updated by the dwarf reader like this? I'm wondering whether
this is something that should be hidden inside a call, like a new
wrapper for cp_add_using.
This is not a big deal.
Tom