Re: [gnome-db] A few bug fix in mysql provider

[Rodrigo Moya <rodrigo gnome-db org>]

On Tue, 2003-10-14 at 03:30, Paisa Seeluangsawat wrote:
> Here you go :-),
> 
looks ok, except for a couple things:
 
> +	/* FIXME: The prototype is:           gda_value_free (GdaValue*)
> +	          but it is called this way:  gda_value_free (GdaValue*, (gpointer) NULL)
> +	          Is this safe?  Doesn't this write NULL other object's memory?
> +	*/
>
remove this comment, please.

>  static GdaRow *
>  fetch_row (GdaMysqlRecordset *recset, gulong rownum)
>  {
> @@ -48,9 +108,13 @@
>  	gint field_count;
>  	gint row_count;
>  	gint i;
> +	/* what for?
>  	unsigned long *lengths;
> +	*/
>
also, here, just remove the function, no need to add those comments,
which just make the code more confusing.
 
>  	mysql_data_seek (recset->mysql_res, rownum);
> -	row = gda_row_new (GDA_DATA_MODEL (recset), field_count);
> -
> -	lengths = recset->mysql_res->lengths;
> +	/* what for?
> +        lengths = recset->mysql_res->lengths;
> +	*/
>
the same here, please just remove code when it's no longer needed, dont
comment it out.

> +		fill_gda_value ((GdaValue*) gda_row_get_value (row, i),
> +			    mysql_fields[i].type,
> +			    mysql_row[i],
> +		/* wrong:   mysql_fields[i].max_length); */ 
>
here, again, what's this comment about? If it's wrong, it shouldn't be
in the code at all.

>  	if (mysql_field->name)
>  		gda_field_attributes_set_name (attrs, mysql_field->name);
>  	gda_field_attributes_set_defined_size (attrs, mysql_field->length);
>  	gda_field_attributes_set_table (attrs, mysql_field->table);
> +	/* gda_field_attributes_set_caption(attrs, ); */
>
here again, if its wrong, remove it, or just fill in the missing data.

> +	/* gda_field_attributes_set_references(attrs, ); */
>
ditto

>  	gda_field_attributes_set_auto_increment (attrs, mysql_field->flags & AUTO_INCREMENT_FLAG);
> +	/* attrs->auto_increment_start */
> +	/* attrs->auto_increment_step  */
> +	gda_field_attributes_set_position (attrs, col);
> +	/* FIXME: 
> +	   Can't use mysql_field->def because,
> +	     - don't know length of the mysql_field->def (needed for binary data)
> +	     - manual says we mysql_field->def isn't set unless we use
> +	       mysql_list_fields().  Then it recommend not to use this function,
> +	       but instead use "show columns from table_name;".
> +
> +	       this is same as "describe table_name;" ?
> +	gda_field_attributes_set_default_value (attrs, ...);
> +	*/
>  
so, you dont set the default value? Then, please add the code to handle
it and send a different patch for it. This patch was about fixing a
leak, and it contains much more things :-) So, please, send a patch for
the leak, and then, when done, a different patch for the default value
implementation.

But adding commented/unfinished code is not an option :-)

cheers