Re: Re : New language support : Vala

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Abderrahim" == Abderrahim KITOUNI <a.kitouni@gmail.com> 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