This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: D language support
> > You right. Initial author is John Demme. I update his patch and add
> > dynamic array support. AFAIK John Demme also assign copyright papers.
Confirmed, I found his copyright assignment on file.
I think you're very close to having something acceptable. If you hurry,
you might even make the gdb 7.1 release :). There are still quite a few
nits here and there, so comments below.
Please also make sure that you patch does not introduce any regression
in our testsuite. Do you know how to test it? Basically, we run the
testsuite before and after applying your patch, and verify that it
does not introduce any new failures by comparing the gdb.sum files
produced before and after. Please also make sure to mention which
architecture this was tested on (typically x86-linux or x86_64-linux).
> gdb/ChangeLog:
> D language support.
> * Makefile.in (SFILES): Add d-lang.c d-valprint.c.
> (COMMON_OBS): Add d-lang.o d-valprint.o.
> (HFILES_NO_SRCDIR): Add d-lang.h.
Small nit on the ChangeLog format: Everything is aligned to the first
tab on the left. For instance:
* Makefile.in (SFILES): Add d-lang.c d-valprint.c.
(COMMON_OBS): Add d-lang.o d-valprint.o.
(HFILES_NO_SRCDIR): Add d-lang.h.
(make sure to use tabs, not spaces)
> * c-alng.c: Make c_emit_char and exp_descriptor_c public.
^^^^^^
> * doc/gdb.texinfo: Add mention about D language support.
This part will need to be approved by Eli, our documentation maintainer.
> * language.c (binop_result_type): Add language_d.
> (integral_type): Add language_d.
> (character_type): Add language_d.
> (string_type): Add language_d.
> (boolean_type): Add language_d.
> (structured_type): Add language_d.
Small note: If you want, you can write the above as:
* language.c (binop_result_type, integral_type, character_type)
(string_type, boolean_type, structured_type): Add language_d.
You're also allowed to use "Likewise", "Ditto", etc.
> +/* Resize unbounded string STR. NEW_SIZE is the new length of STR. */
^^^ Missing second space after period.
You need to describe what this function does when NEW_SIZE is zero.
> +static void
> +str_resize (unbounded_string* str, size_t new_size)
> +{
> + int pos = str->pos - str->str;
> + if (new_size == 0)
Formatting nit: Can you add an empty line after declaration blocks.
It's sort of an unwritten convention that most developers follow.
Can you fix that in all of your patch?
> + new_size = strlen(str->str) + 1;
> + str->str = xrealloc(str->str, new_size);
You need to add a space before the opening parenthesis in function calls.
Can you fix your code throughout?
> +/* Extract identifiers from MANGLED and append it to OUTPUT.
> + Return 1 on success or -1 on failure. */
> +static int
> +extractidentifiers (unbounded_string* output, unbounded_string* mangled)
Can you rename your function to extract_identifiers, please?
Entity name conventions for GNU projects in C is to put underscores
between words... Likewise elsewhere in your patch.
> +{
> + long i = 0;
> + while (isdigit(*mangled->pos))
> + {
> + i = strtol(mangled->pos, &mangled->pos, 10);
> + if (i == 0 || i == LONG_MAX || i == LONG_MIN)
> + return -1;
Aren't you supposed to check errno when the returned value is LONG_MAX
and/or LONG_MIN. Otherwise, you exclude these values as invalid.
Perhaps these values can never happen, and your test is sufficient,
but a comment would be welcome.
> +/* Extract and demangle type from ID and append it to DEST.
> + Return 1 on success or -1 on failure. */
Usual conventions are to return non-zero on success, and zero
otherwise. I recommend you follow this practice, as this allows
you to write:
if (!extract_type_info (dest, id))
return 0;
> + case 'n': append(dest, "none"); return 1;
This is not a blocking comment, meaning that this is not a request
to fix before your patch gets accepted, but I'm starting to think
that the way you use your unbounded_string is to build the output
before printing it. We typically would use a "ui_file" stream,
for that. In your case, you want a "memory" ui-file. Have a look
at ui-file.h, and in particular at mem_fileopen, ui_file_write,
and ui_file_xstrdup. That should provide you all the infrastructure
you need to get rid of this local "unbounded_string" type.
I'll give you the choice: Look at this as part of this patch, or
promise me that you'll look at it soon after this patch is in our tree.
> +/* D specific symbol demangler. SYMBOL_ is a mangled name.
^^^ missing hyphen. ^^^ Missing second space after period.
> + OPTIONS is demangler options. Return demangled name or NULL on failure. */
^^^ Missing seconc space.
For the routine that implement a given function of a known "vector",
You don't need to repeat the description of that function. The description
should already be provided where the function pointer is declared,
and repeating it here can potentially leead to inconsistencies.
So I'd rather you put a comment like the following:
/* Implements the la_demangle language_defn routine for language D. */
> +char*
Missing space: char *
> +d_demangle (const char* symbol_, int options)
^^^^^^
Could you use something other than a trailing underscore in your
parameter name. I've seen people use symbol1, for instance. A more
meaningful name would be ideal, but if it makes it too long, then
go for symbol1.
> + unsigned char isFunc = 0;
^^^^^^ is_func.
> + return strdup("D main");
^^^^^^ use xstrdup.
> + symbol = xstrdup(symbol_);
I don't really understand why you are duplicating the symbol string,
here...
> + mangled.len = strlen(symbol);
> + mangled.str = symbol;
> + mangled.pos = symbol;
You're breaking a bit your abstraction by setting up your unbounded
string manually. This should encourage you to look at ui_files...
> + output.len = 2;
> + output.str = xmalloc(output.len);
> + output.pos = output.str;
> + if (symbol == strstr(symbol, "_D"))
Isn't just simpler to do:
if (symbol[0] == '_' && symbol[1] == 'D')
Or:
if (strncmp (symbol, "_D", 2) == 0
?
> +/* Table mapping opcodes into strings for printing operators
> + and precedences of the operators. */
> +const struct op_print d_op_print_tab[] =
Can you make this static?
> +const struct language_defn d_language_defn =
> +{
Same for this one? I'm not entire sure, since it looks like we're not
making static anywhere, but I don't see why not.
> +/* Detect dynamic array and print his contents. Returns the number of string
^^^ its contents.
Also, missing second space after the period.
> + characters printed or -1 if TYPE not dynamic array. */
> + if (TYPE_NFIELDS (type) == 2
> + && TYPE_CODE (TYPE_FIELD_TYPE (type, 0)) == TYPE_CODE_INT
> + && strcmp (TYPE_FIELD_NAME (type, 0), "length") == 0
> + && strcmp (TYPE_FIELD_NAME (type, 1), "ptr") == 0)
Should you check the TYPE_CODE (type), here? I assume TYPE_CODE_STRUCT?
Hmmm, actually, coming back to here after having read the rest of
the patch, I see that this check has already been made. You should
probably make a note of this in your function description. I propose
the new following description:
/* Assuming that TYPE is a TYPE_CODE_STRUCT, verify that TYPE is
a dynamic array, and then print its value to STREAM. Return
the number of string characters printed, or -1 if TYPE is not
a dynamic array. */
You might also want to describe all the other parameters more in detail.
> +/* Print data of type TYPE located at VALADDR (within GDB), which came from
> + the inferior at address ADDRESS, onto stdio stream STREAM according to
> + OPTIONS. The data at VALADDR is in target byte order.
> +
> + If the data are a string pointer, returns the number of string characters
> + printed. */
Same comment as above. Since the semantics of this routine is well known
and defined elsewhere, just say that it implement the la_val_print
routine for language D.
> + if (ret != -1)
> + break;
> + default:
> + ret = c_val_print (type, valaddr, embedded_offset, address, stream,
> + recurse, options);
This is surprising me a little. Don't you have plain struct types
that are not dynamic arrays? Right now, the code seems to indicate
that you don't expect any. Or maybe I missed something?
--
Joel