Re: D language support

[Joel Brobecker <brobecker@adacore.com>]

> It allows to call allocation of memory less often. This way chose John
> Demme. It effective way, when we need allocate many unbounded buffers
> with big size.

Are you sure this is a problem in practice? Have you made some measurement
that string allocation/deallocation for D identifiers are having
a noticeable  performance impact on GDB operations?

> But in ours case we need only on small buffer. 
> I prefer another way - just allocate one buffer with size bigger than
> worst case.

If you can prove that you can never ever exceed that worse case
scenario, then that would be fine, as long as you provide a comment
justifying this hard-coded limitation and how it was computed.
Otherwise, if some code, or some user command can lead to overflow,
however whimsical it might be, then you need to switch to a dynamic
approach.  The GNU Coding Standards explicitly warn against using
abitrary limits.

Which approach you chose is no longer important to me. The suggestions
were meant to be helpful, but it's not a huge amount of code and it's
going to be "your" maintenance problem - if any.

> 	* c-lang.c: Make c_emit_char and exp_descriptor_c public.
> 	* c-lang.h: Add declaration c_emit_char and exp_descriptor_c.

Please change the above to:

        * c-lang.c (c_emit_char, exp_descriptor_c): Make public.
        * c-lang.h (c_emit_char, exp_descriptor_c): Add declaration.

> 	* symtab.c Include d-lang.h.
                ^^^^ missing colon.

> 	* testsuite/gdb.base/default.exp: fix "set language" test.
                                         ^^^ Fix

> +static char* out_str;
> +static char* out_pos;
> +static char* mangled_str;

I prefered it when you had a type, encapsulating the whole bounded
string concept. You can always make it static if you think it helps.

If you are going to use a hard-coded limit, please use a constant
or a macro:

  #define MAX_BLA_BLA_BLA
  struct bounded_string
  {
    char buf[MAX_BLA_BLA_BLA];
    char *buf_pos;
  };

This is important, because I'm seeing "magic" constants in your code,
such as:

> +  if (size < 1024) {
> +    memcpy (out_pos, src, len);
> +    out_pos += len;

A few comments on the code above:

  . Formatting: The '{' should be on the next line.
  . No litteral constant, use MAX_BLA_BLA_BLA.
  . You do nothing when there is an overflow; This is bad, because
    this will at best lead to a mysterious error message, and at worst
    a silent malfunction.  You need to error() out.  If there is no
    valid situation where the overflow should ever happen, then
    it is internal_error() that should be called.

> +static int
> +extract_identifiers ()
                      ^^^^ (void)

> +static int
> +extract_type_info ()
                    ^^^^^ same here

> +  mangled_str = (char *) symbol;

Outch! You are bypassing the "const", preventing potentially useful
compiler warnings. Isn't there a way to avoid this?

> +  out_str = xmalloc(1024);
> +  out_pos = out_str;

If you have a static unbounded_string, you could just allocate it
once, instead of everytime to start demangling (in other words,
declare an array of char instead of a char pointer).

> +    } else if (mangled_str == strstr (mangled_str, "__Class_"))
> +      mangled_str += 8;
> +    else if (mangled_str == strstr (mangled_str, "__init_"))
> +      mangled_str += 7;
> +    else if (mangled_str == strstr (mangled_str, "__vtbl_"))
> +      mangled_str += 7;
> +    else if (mangled_str == strstr (mangled_str, "__modctor_"))
> +      mangled_str += 10;
> +    else if (mangled_str == strstr (mangled_str, "__moddtor_"))
> +      mangled_str += 10;
> +    else if (mangled_str == strstr (mangled_str, "__ModuleInfo_"))
> +      mangled_str += 13;

Use strncmp, it will be more efficient.

> +  c_printchar,		/* Print a character constant */
> +  c_printstr,		/* Function to print string constant */
> +  c_emit_char,		/* Print a single char */
> +  c_print_type,		/* Print a type using appropriate syntax */
> +  c_print_typedef,	/* Print a typedef using appropriate syntax */
> +  d_val_print,		/* Print a value using appropriate syntax */
> +  c_value_print,	/* Print a top-level value */
> +  NULL,			/* Language specific skip_trampoline */
> +  "this",		/* name_of_this */
> +  basic_lookup_symbol_nonlocal, /* lookup_symbol_nonlocal */
> +  basic_lookup_transparent_type,/* lookup_transparent_type */
> +  d_demangle,		/* Language specific symbol demangler */
> +  NULL,			/* Language specific class_name_from_physname */
> +  d_op_print_tab,	/* expression operators for printing */
> +  1,			/* c-style arrays */
> +  0,			/* String lower bound */

If you use English-style comments, then you need to start sentences
with a capital letter, and end them with a period.  For instance:

  /* Print a character constant.  */

I haven't double-checked, but I think some of us just put the name
of the struct component in the comment.  That way, someone modifying
the profile of one of these routines could simply grep of that
component name to find implementations of that routine.

> +  else if (current_language->la_language == language_d)
> +    {
> +      demangled_name = d_demangle (name, 0);
> +      if (demangled_name)
> +	{
> +	  mangled_name = name;
> +	  modified_name = demangled_name;
> +	  make_cleanup (xfree, demangled_name);
> +	}
> +    }

Can you move this back to after 'else if (lang == language_java)'?

-- 
Joel