This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Re : New language support : Vala


>>>>> "Abderrahim" == Abderrahim KITOUNI <a.kitouni@gmail.com> writes:

Sorry about the long delay on this.

As a general rule, please send a patch ping message after a week or
two without a response.  This is currently the best way to ensure that
your patch does not fall through the cracks.

Tom> I don't know whether your FSF paperwork has gone through.  I don't
Tom> have a way to look that up any more; maybe someone else could find
Tom> out.

Abderrahim> Yes, I received the letter.

Ok.  The important bit for us is knowing when the FSF says that you
are all set up.

Tom> +  target_read_string (value_as_address (name), &type_name, 100, NULL);

Tom> That '100' looks odd.  It would probably be better to use the new
Tom> string-reading code that Thiago put in; see valprint.c:read_string.

Abderrahim> I didn't find a better solution (read_string returns a
Abderrahim> gdb_byte* and I don't know how to convert it).

The result will be a string in the target encoding.  You can convert
it to the host encoding, assuming that is what you want, using the
functions declared in charset.h.

The patch is looking pretty good.  You'll need to update it to work
with cvs; some incompatible changes have gone in.

A few specific notes:

+  char *_name;

Don't start symbols with an underscore.

+  name = call_function_by_hand (g_type_name, 1, &gtype);

You may want to find a way to do this without an inferior call.
First, this won't work for core files.  Second, if the user puts a
breakpoint in g_type_name, something weird will happen.

+const struct op_print vala_op_print_tab[] =

Should be static.

+static struct field *
+vala_type_get_fields (struct type *type, int *nfields)
+{
+  int offset = 0;
+
+  if (TYPE_CODE (type) != TYPE_CODE_STRUCT)
+    /* error */
+    return NULL;

This can return NULL without setting *nfields, but...

+		  fields = vala_type_get_fields (real_type, &nfields);
+		  privfields = vala_type_get_private_fields (real_type, &nprivfields);
+		  if (nfields + nprivfields > 0)

... this code does not check that.

vala_type_get_private_fields has the same bug.

I didn't look to see if this condition is checked before the call.  If
so then you can just call internal_error or error rather than return
NULL.


+      if (VALA_TYPE_HAS_PRIV (type))
+	{
+	  offset = 2;
+	}

Remove the braces here.

+      else if (TYPE_CODE (real_type) == TYPE_CODE_INT &&
+	       streq (TYPE_NAME (real_type), "char"))

Put "&&" at the start of the line, not the end.

+	fputs_filtered (TYPE_CODE (type) == TYPE_CODE_STRUCT ?
+			"struct " : "enum ", stream);

Ditto the "?"

+      /* Don't print typedefs that just remove the '_'.  */
+      if (streq (SYMBOL_NATURAL_NAME (new_symbol), TYPE_TAG_NAME (type) + 1))
+	return;

It seems like this should actually check for the "_".

+  if (TYPE_CODE (type) == TYPE_CODE_PTR &&

"&&" position.

+      TYPE_CODE (type =
+		 check_typedef (TYPE_TARGET_TYPE (type))) == TYPE_CODE_STRUCT)

GNU style prohibits embedded assignments like this.

The linespec patch looks decent.  I am not sure we want to do it this
way, though.  I think a new language function would be preferable.

One nit:

+      result = strdup ("g_");

Should be xstrdup.

Also, the new style is to put a static function above its users, and
then not have a forward declaration for it.  This is simpler to
maintain.

Too bad the Python support isn't further along.  I'd like to see a new
language supported purely using Python; this one would have been a
good choice :-)

I know it is a pain, but this really needs test cases and
documentation -- documentation for the users, and test cases so the
gdb developers can test it occasionally.  ("Occasionally" since I
assume most of us won't have the vala compiler installed.)

Tom


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]