From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24957 invoked by alias); 9 Jan 2010 14:23:48 -0000 Received: (qmail 24936 invoked by uid 22791); 9 Jan 2010 14:23:46 -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; Sat, 09 Jan 2010 14:23:41 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 1E1C12BAB54; Sat, 9 Jan 2010 09:23:39 -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 Bal1ui4a5AZY; Sat, 9 Jan 2010 09:23:39 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 2209B2BAB66; Sat, 9 Jan 2010 09:23:38 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id CA936F595E; Sat, 9 Jan 2010 18:23:27 +0400 (RET) Date: Sat, 09 Jan 2010 14:23:00 -0000 From: Joel Brobecker To: Mihail Zenkov Cc: gdb-patches@sourceware.org Subject: Re: D language support Message-ID: <20100109142327.GC2007@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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20100109082830.dd984de8.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/msg00214.txt.bz2 > > 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