From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5849 invoked by alias); 30 Dec 2009 12:56:21 -0000 Received: (qmail 5835 invoked by uid 22791); 30 Dec 2009 12:56:18 -0000 X-SWARE-Spam-Status: No, hits=-2.5 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; Wed, 30 Dec 2009 12:56:12 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 5A63F2BAC0B; Wed, 30 Dec 2009 07:56:10 -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 gu6W2MTN+ZLF; Wed, 30 Dec 2009 07:56:10 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 6CAF02BAB78; Wed, 30 Dec 2009 07:56:08 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 87E74F5937; Wed, 30 Dec 2009 13:55:39 +0100 (CET) Date: Wed, 30 Dec 2009 12:56:00 -0000 From: Joel Brobecker To: Mihail Zenkov Cc: gdb-patches@sourceware.org Subject: Re: D language support Message-ID: <20091230125539.GF2788@adacore.com> References: <20091224015617.7092112b.mihai.zenkov@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20091224015617.7092112b.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: 2009-12/txt/msg00436.txt.bz2 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