From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28311 invoked by alias); 15 Jan 2010 05:10:44 -0000 Received: (qmail 28302 invoked by uid 22791); 15 Jan 2010 05:10:42 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 15 Jan 2010 05:10:37 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 11CCF2BAB97; Fri, 15 Jan 2010 00:10:35 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id VOcLGSEKb5CL; Fri, 15 Jan 2010 00:10:35 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 043692BAB8A; Fri, 15 Jan 2010 00:10:33 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 39912F5970; Fri, 15 Jan 2010 09:10:24 +0400 (RET) Date: Fri, 15 Jan 2010 05:10:00 -0000 From: Joel Brobecker To: Mihail Zenkov Cc: gdb-patches@sourceware.org Subject: Re: D language support Message-ID: <20100115051024.GA2786@adacore.com> References: <20091224015617.7092112b.mihai.zenkov@gmail.com> <20091230125539.GF2788@adacore.com> <20100109082524.263bcb17.mihai.zenkov@gmail.com> <20100109082830.dd984de8.mihai.zenkov@gmail.com> <20100109142327.GC2007@adacore.com> <20100113064026.14f75ff2.mihai.zenkov@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20100113064026.14f75ff2.mihai.zenkov@gmail.com> User-Agent: Mutt/1.5.20 (2009-06-14) Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2010-01/txt/msg00398.txt.bz2 > 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