Re: D language support

[Joel Brobecker <brobecker at adacore dot com>]

Mihail,

Not bad :). There were a lot of little things, so I focused on the obvious
(procedural) ones for now, and I will review the contents of your code
again more carefully on the next revision.

Before I go into the details, I have a question, though: Are you the sole
author of this patch? I am asking because there are really two distinctive
coding styles used in this case. One is very different from the style
used in GDB (and other GNU projects written in C), and the other one
is pretty spot on in terms of the coding style.  Hard to believe that
the same person wrote both parts! It's very important to know, because
only the author may assign the copyright to the FSF (IANAL, but I believe
this to be true).

Procedural remark: All patches should be accompanied by a ChangeLog
entry. You can have a look at gdb/ChangeLog for more info on how to
write one. Also, I believe the GNU Coding Standards (GCS, see
http://www.gnu.org/prep/standards/standards.html) explains in more
details how to write an entry.

The documentation is reviewed by Eli Zaretski.  So this review does not
include that part of your patch.

> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 348d853..768adfe 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in

You also need to update SFILES to add d-lang.h.

> -	ada-valprint.o c-valprint.o cp-valprint.o f-valprint.o m2-valprint.o \
> +	ada-valprint.o c-valprint.o cp-valprint.o d-valprint.o f-valprint.o m2-valprint.o \

Can you split this line in two, please. We avoid lines exceeding 75 to 78
characters.

> +const struct language_defn d_language_defn =
> +{
> +  "d",				/* Language name */
> +  language_d,
> +  range_check_off,

It's annoying, IMO, to have to declare this struct inside c-lang,
even if D, as I understand it, is close to C, especially since you
introduce a new file d-lang.c.  I noticed only two entities that
are currently static:
  1. exp_descriptor_c
  2. c_emit_char
I think it's OK if both are made public (we can add declarations in
c-lang.h), so that they can be used from d-lang.c.  That way, c-lang.c
does not get affected.

> +/* C language support routines for GDB, the GNU debugger.
      ^^ D

> +   Copyright 1992, 1993, 1994, 1995, 1996, 1998, 1999, 2000, 2002, 2003, 2004
> +   Free Software Foundation, Inc.

Are the copyright years really accurate. I was expecting only 2009.
I suspect a copy/paste error, given the copy/pasto above.

> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 2 of the License, or
> +   (at your option) any later version.
[...]
The copyright header should mention version *3* of the GPL. You can grab
a correct header from one of the other files (eg top.c).

GDB follows the GNU Coding Standard. Please have a quick read at
the section detailing how to write C programs, at least.
And please bear with me while I point out some of the deviations
("Formatting:")...

> +typedef struct {
> +  size_t len;
> +  char* str;
> +  char* pos;
> +} String;

Formatting: The curly brace should be at the beginning of the next line:

    typedef struct
    {
      [...]
    } String;

But also, the entity names should be all lower case with underscores
between words.  This looks like some kind of unbounded_string, so
you could name it "unbounded_string", for instance.

> +static size_t str_left(String* str) {
> +  return (str->len - (str->pos - str->str));
> +}

Three formatting nits:

function names for definitions (not declarations) should be at the
beginning of the line, and curly braces should be on the next line.
Also, you need to add a space between the open parenthesis.

    static size_t
    str_left (String* str)
    {

Also, all functions must be documented, even when the purpose of
the function looks trivial.  A short comment at the start of the
function is usually enough if the function is simple.  Please have
a look at other functions for an idea of how the code is documented.
For instance;

    /* Execute the line P as a command, in the current user context.
       Pass FROM_TTY as second argument to the defining function.  */
    
    void
    execute_command (char *p, int from_tty)

I hope you're not getting discouraged. This is all a formality, but
it really helps the maintenane if everyone follows a consistent style.

> +static void str_resize(String* str, size_t new_size) {
> +  int pos = str->pos - str->str;
> +  if (new_size == 0)

In addition to the above, this code shows another tiny deviation:
We put an empty line between the declaration block and the statements.
Thus:

    int pos = ...;
    
    if (new_size == 0)

> +  memcpy(str->pos, src, i);

Please put a space between the function name and the open parenthesis.
I think the rule is there is always a space before an open parenthesis
unless the previous character is also an open parenthesis, but do double
check in the GNU Coding Standards (I only know the usage for sure).

> +  while (isdigit(*mangled->pos)) {

Formatting; { goes to the next line. Same for "for", "if", etc.

> +  // Extract the type info:

I like the fact that you are commenting your code. This is really
helpful. However, GDB follows ISO C 90, which means that you need
to write your comments using /* and */.  Also, I should mention that,
except for very short comments such as the below:

> +    // array, static array, dynamic array:

Comments are expected to be full sentences, starting with a capital
letter, and ending with a dot.  We are also expected to put 2 spaces
after each dot. For instance:

   /* I am doing this because I must.  Otherwise, it does not work.  */

(in particular, notice the two spaces before the */).

> +  case 'A': case 'G': case 'H':

I'd rather you had each case on a separate line.  I don't know if
this is GCS or not, but it's been the usage in GDB AFAIK.

> +    // pointer:
> +  case 'P':

In a case like this, we prefer if the short comment in on the same line
as the "case" statement.  This makes it easier to determine what the
comment applies to.

> +  // typeinfo, error, instance:
> +  case '@': return extractidentifiers(dest, id); // BUG: is this right?

Do you think you could investigate the question above? I'd rather we
have a look now if it's not too much work, now that we're looking at
the code, rather than letting it in, and possibly really never look
at this again.

> +  default: append(dest, "unknown"); return 1;

I would prefer, for the default case, that you put each statement
in its own line:
    
    default:
      append (dest, "unknown");
      return 1;

I would actually prefer it if you used this style throughout, for
instance where you had a series of them:

    +  case 'h': append(dest, "ubyte"); return 1;
    +  case 's': append(dest, "short"); return 1;
    +  case 't': append(dest, "ushort"); return 1;

But I can see why you like everything on one line and I would not
personally insist. I don't know what the other maintainers think.
I do think it would look more consistent with the rest, though.

> +  char* symbol;
> +  //printf("%s: ", symbol);

Generally speaking, commented out code should not be left without an
explanation. In this case, it just looks like an old trace left by
accident? Can you just remove it (and all other such traces)?

If you occasionally need those traces to diagnose an issue, you might
want to think about providing a debug setting that can be used to turn
some traces on.  Have a look at "set debug infrun" for instance, and
how it is implemented in infrun.c.  If you do add traces, please
consider making them a little more intelligible than NULL1, NULL2 and
NULL3...

> +  } else if (strcmp(symbol_, "_Dmain") == 0) {
> +    return strdup("D main");
> +  }

We do not use the curly braces when the block only have one statement.

    else if (strcmp (symbol_, "_Dmain") == 0)
      return [...];

> +/*****************************
> + D Language stuff
> +******************************/

This comment is really superfluous and should be removed.

> @@ -8083,7 +8086,7 @@ dwarf_decode_lines (struct line_header *lh, char *comp_dir, bfd *abfd,
>                  else
>                    {
>                      fe = &lh->file_names[file - 1];
> -                    if (fe->dir_index)
> +                    if (fe->dir_index && lh->include_dirs != NULL)
>                        dir = lh->include_dirs[fe->dir_index - 1];
>                      if (!decode_for_pst_p)
>                        {

This change needs to be explained and submitted as a separate patch.

> @@ -483,6 +485,15 @@ symbol_find_demangled_name (struct general_symbol_info *gsymbol,
>    if (gsymbol->language == language_unknown)
>      gsymbol->language = language_auto;
>  
> +  if (gsymbol->language == language_d
> +      || gsymbol->language == language_auto) {
> +    demangled = d_demangle(mangled, 0);
> +    if (demangled != NULL) {
> +      gsymbol->language = language_d;
> +      return demangled;
> +    }
> +  }

Would you mind moving this to the end of the function. I don't understand
how specific the mangling is done for D, but I'm concerned that it might
successfully demangle non-D symbol names. What do you think?

-- 
Joel