From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8309 invoked by alias); 24 Feb 2009 18:58:08 -0000 Received: (qmail 8300 invoked by uid 22791); 24 Feb 2009 18:58:07 -0000 X-SWARE-Spam-Status: No, hits=-2.3 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mx2.redhat.com (HELO mx2.redhat.com) (66.187.237.31) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 24 Feb 2009 18:57:58 +0000 Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26]) by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n1OIvtDs014278; Tue, 24 Feb 2009 13:57:55 -0500 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx2.corp.redhat.com (8.13.1/8.13.1) with ESMTP id n1OIvskY016327; Tue, 24 Feb 2009 13:57:55 -0500 Received: from opsy.redhat.com (vpn-12-148.rdu.redhat.com [10.11.12.148]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id n1OIvrnn028758; Tue, 24 Feb 2009 13:57:54 -0500 Received: by opsy.redhat.com (Postfix, from userid 500) id 09FCE508295; Tue, 24 Feb 2009 11:57:51 -0700 (MST) To: Abderrahim KITOUNI Cc: gdb-patches@sourceware.org Subject: Re: Re : New language support : Vala References: <3d6b0edb0902070607x29177016m48a40bd198b88f7e@mail.gmail.com> <3d6b0edb0902090505m6bcb142crab53b0f860535ff4@mail.gmail.com> <3d6b0edb0902182352p7143c29clb382aa5602720463@mail.gmail.com> From: Tom Tromey Reply-To: tromey@redhat.com Date: Tue, 24 Feb 2009 19:58:00 -0000 In-Reply-To: <3d6b0edb0902182352p7143c29clb382aa5602720463@mail.gmail.com> (Abderrahim KITOUNI's message of "Thu\, 19 Feb 2009 08\:52\:11 +0100") Message-ID: User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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-02/txt/msg00471.txt.bz2 >>>>> "Abderrahim" == Abderrahim KITOUNI writes: Abderrahim> Here is the first part of the patch, comprising what I Abderrahim> think is changes that don't affect anything else, I'll Abderrahim> post the remaining things later. Thanks. Here's a detailed review, mostly about various formatting nits and the like. We're sticklers for proper formatting; and the GNU style is a bit weird and unfamiliar to most folks. I think there are one or two more substantive comments in here as well. Overall this patch is looking pretty good. It is worth mentioning where you built and tested this. Also, you may want to consider test suite additions for the vala support... I don't know whether your FSF paperwork has gone through. I don't have a way to look that up any more; maybe someone else could find out. + * gdb/buildsym.c (start_subfile): Deduce language from filename for Vala + aswell (as it's compiled to C). That first line looks too long. "aswell" is missing a space. In general you don't need to explain the why of a change -- e.g., the parenthetical comment is not needed. + * gdb/c-lang.c (c_emit_char): Make it non static so it can be used from + Vala. Or, e.g., just "Now non-static" is fine here. + vala-lang.c vala-print.c\ Missing space before the \. + vala-lang.o vala-print.o\ Likewise. - || subfile->next->language == language_fortran)) + || subfile->next->language == language_fortran + || subfile->next->language == language_vala)) Extra space before the ==. vala-lang.c, vala-lang.h, and vala-print.c all need copyright headers. You can copy one from just about any other .c file in gdb; just make sure to update the first line (which describes the purpose of the file) and the years. +static CORE_ADDR vala_skip_trampoline (struct frame_info *, CORE_ADDR); +static struct value *vala_value_of_this (int); +static struct type *vala_lookup_transparent_type (const char *); +static char *vala_demangle (const char *, int); I think our current recommended style is to try to order new code so that forward declarations are not needed for static functions (except in the case of mutual recursion, of course). This affects vala-print.c as well. +/* defined in c-lang.c */ +extern const struct op_print c_op_print_tab[]; Two notes here. First, in the GNU style, comments are full sentences, starting with a capital letter and ending with a period followed by 2 spaces. I mention this because there are a number of comments that need updating. Second, I think it is better to simply drop the comment and move this declaration to c-lang.h. +const struct language_defn vala_language_defn = { + "vala", /* Language name */ Move this toward the end of the file (avoiding the need for forward declarations). Also the "{" goes on its own line; see c-lang.c. +extern void +_initialize_vala_language (void) +{ + add_language (&vala_language_defn); +} By convention the _initialize function comes at the end of the file. Also, drop the "extern" -- this applies in several places, e.g. vala-print.c. There's no need to have extern on a definition. +static char * +vala_demangle (const char *mangled, int options) The comment before this function should describe the arguments and the expected return values. Other functions in this file need header comments as well. + char *class_name = strdup (""); Use xstrdup in gdb. + part = strtok (name, "_"); I think you have to #include "gdb_string.h" to portably use strtok in gdb. I am not sure if you want to be using safe-ctype.h or ctype.h. + if (part + strlen (part) < name + strlen (mangled) && + (streq (part, "new") || streq (part, "real"))) + { + /* skip class name + '_' + "new" or "real" */ + method_name = strdup (mangled + (part - name) + 1 + strlen (part)); + } When there is a single statement in the body of an if (or else or while or whatever), the GNU style omits the braces. This shows up in a few places. + if (streq (func_name + (strlen (func_name) - 9), "_get_type") || + streq (func_name + (strlen (func_name) - 4), "_ref") || + streq (func_name + (strlen (func_name) - 6), "_unref")) Operators, in this case ||, go at the start of a line, not the end. This applies in a few places, e.g. vala_value_print. You may want to hoist those repeated strlen calls into a local variable. +/* Returns the real (runtime) type of val */ In the GNU style, when a comment refers to the value of a variable, the variable's name is capitalized. + target_read_string (value_as_address (name), &type_name, 100, NULL); That '100' looks odd. It would probably be better to use the new string-reading code that Thiago put in; see valprint.c:read_string. Also, I think the code should properly handle errors. +#define VALA_TYPE_NAME(type) ((TYPE_TAG_NAME (type) != NULL && \ + TYPE_TAG_NAME (type)[0] == '_') ? \ + TYPE_TAG_NAME (type) + 1 : TYPE_TAG_NAME (type)) I think it would be good if all the macros in vala-lang.h had comments explaining their purpose and arguments. +#define VALA_TYPE_IS_CLASS(type) (TYPE_FIELDS (type) != NULL && \ + (streq (TYPE_FIELD_NAME(type, 0), "parent_instance") \ + || streq (TYPE_FIELD_NAME(type, 0), "g_type_instance"))) +#define VALA_TYPE_BASE_CLASS(class) (check_typedef(TYPE_FIELD_TYPE (class, 0))) +#define VALA_TYPE_HAS_PRIV(type) (TYPE_NFIELDS (type) >= 2 && \ + streq (TYPE_FIELD(type,1).name, "priv")) These all are missing spaces before "(" in the bodies. +extern void +vala_print_typedef (struct type *type, struct symbol *new_symbol, + struct ui_file *stream) +{ + +}; It seems strange to have an empty function for this. If this is what you intended, it deserves a comment explaining why. Otherwise, I suggest removing it and using the C function. Tom