Ticket #3 (closed defect: fixed)

Opened 9 months ago

Last modified 2 months ago

Construction failure raises floating point exception

Reported by: Mike Wey Assigned to: Mike Wey
Priority: critical Milestone: RC 1
Component: gtkd - classes Version: TRUNK
Keywords: Constructors divide zero exception Cc:

Description

Because of the 'zero = zero / zero' in the constructors, calling Button.getImage() on a button without an image generates a 'Floating point exception'. There are probably more methods like this that should return null.

Example:

import gtk.MainWindow;
import gtk.Button;
import gtk.GtkD;
import gtk.Image;

class ButtonTest : MainWindow
{
	this()
	{
		super("Button Test");
		setDefaultSize(50, 30);
		Button btn = new Button();
		btn.setLabel("Test");
		Image image = cast(Image)btn.getImage();
		add(btn);
		showAll();
	}
}

void main(char[][] args)
{
	GtkD.init(args);
	new ButtonTest();
	GtkD.main();
}

Change History

(in reply to: ↑ description ; follow-up: ↓ 4 ) 12/18/07 22:31:25 changed by JJR

I noticed this too in one constructor... and now I see that the divide by zero is used in others too via version(noAssert). I'm not sure why this form of exception is used to stop the program. A properly named catchable exception might be a better replacement? I suppose the divide by zero is also catchable, though?

(follow-up: ↓ 3 ) 12/19/07 01:16:25 changed by JJR

  • component changed from DSSS Support to gtkd - classes.
  • milestone changed from Pre-release 7 to Pre-release 8.

(in reply to: ↑ 2 ) 12/19/07 04:37:00 changed by kaarna

Replying to JJR:

This issue has already been discussed in the forums in length.

My suggested fix for this whole issue can be found here.

And my final conclusions that have a similar example as this tickets getImage() can be found here.

To sum it up briefly: We can't use exceptions here because they will terminate the program if they aren't catched. (One should be able to call getImage() on a button without checking for exceptions.) If the image isn't there getImage should return null. The easiest and IMO the best solution is to put the following in to every constructor:

this(GdkPixbuf?* gdkPixbuf) {

if( gdkPixbuf is null ) {

this = null; return;

} else do stuff...

}

See the this = null. That will set the created D object to be null if the C object was null. If we'd have this everywhere, then it would all just work, and you only needed to check for null when using your gtkD objects. That's the way it's done in every other language (which have exceptions too, but it's not good to use here...) and that's the way it should be. Read my posts linked above for the reasoning.

And this is propably easy to do in gtkwrap. Just replace the part where the current version(noassert) and divide by zero exceptions are with the proposed this = null thing. I'm not going to discuss this any further. :)

(in reply to: ↑ 1 ) 12/19/07 04:45:33 changed by kaarna

Replying to JJR:

(Sorry, replied to the wrong comment there, but I think it's clear anyway...)

One further point :)

Exceptions should be used when something is exceptional i.e. something goes horribly wrong unexpectedly. This is not one those situations. Checking for null could be used for minor issues like this instead. (Other form of error handling could be used too, for example checking the output of some isError() method or isNull() method, but checking for null is the standard way of doing it. You could still have some kind of logging of errors, but that could be separate and done later.)

12/19/07 04:58:06 changed by kaarna

  • priority changed from minor to major.

12/19/07 06:22:40 changed by kaarna

  • priority changed from major to critical.

12/19/07 22:31:07 changed by JJR

Yes, I went back and read some of the posts. It certainly has been discussed. I kind of jumped the gun. :)

If the null is going to be assigned to the object, at the very least I think there should be a way to determine what the error was... a mere null won't do that, so I agree that at some point error logging should take place if you want to go this route (null assignment), or else this may turn out to be a royal pain figuring out why an object is null. (I want to qualify, though, that I'm not an expert here).

Concerning exception handling, however, what is and is not exceptional situation seems to be a relatively arguable case. GtkD is based on gtk+ in which nulls are likely tested for after many different calls in normal C programming practice (ick!). Exceptions remove a lot of the if/then clauses for null testing inherent in the old C style.

Furthermore, I don't think the use of exceptions means that the program necessarily must exit... does it? The way it works right now (halt on hardware error or assert) exits the program. Instead these could merely throw an exception that could be handled by the calling program in which it is very likely that the calling party should investigate why the underlying gtk+ function returned a null object in the first place. In effect this is translating nulls to oop exception handling. (Granted, I don't really have much experience with using gtk+ so I don't know how regular an occurrence "null" returns are, so take this opinion with a grain of salt)

So, in conclusion, do exceptions /necessarily/ mean termination of the program? If not why are they a disadvantage compared to null returns? If the argument is "everybody else does it the "null-return" way", does that make the argument valid? I'd prefer to follow the cleanest way, not necessarily the most popular or the easiest to implement. :)

(follow-up: ↓ 10 ) 12/20/07 11:16:58 changed by Pse

  • summary changed from Floating point exception to Construction failure raises floating point exception.

Being used to the way GTK works, I'd say returning 'null' would be the expected behaviour by most GTK programmers. This is a good thing because it flattens the learning curve for most GTK programmers. However, as JJR pointed out, returning a 'null' does not really tell you what the error was. My take on this matter is the following:
Current behaviour is broken and should be changed. ASAP. So options are:
a) Return 'null'. Let the user figure out what happened, GTK provides several methods to do this that could be wrapped (basically what kaarna said).
b) Raise an exception if and only if the user explicitly requested to construct an object and that failed. Do not raise exceptions inside wrapped gtk members (i.e. getImage for instance), instead return 'null'.

Option 'b' is clearly more difficult to implement, as it'd require two things:
1) Change each constructor to raise an exception in case construction fails. And a valid exception, not an ugly 'zero/zero' hack.
2) Change each member returning a newly wrapped object to handle 'null' values returned by GTK functions as follows:

public Widget getImage()
{
    // GtkWidget* gtk_button_get_image (GtkButton *button);
    // return new Widget( gtk_button_get_image(gtkButton) ); /* Current behaviour */
    GtkWidget* p = gtk_button_get_image(gtkButton);
    return p ? new Widget(p) : null
}

I favour option 'b' over option 'a', but any will do over the current implementation, which needs to be fixed as soon as possible.

12/20/07 11:58:17 changed by Pse

Forgot to say.. There's no point in going the exception way if no specialized exceptions are raised. That is, the advantage of exceptions (over null objects) lies in letting the user know (and handle) what went wrong. If there's only one thing that can go wrong, then we might as well give the user a null object and be done with it. Given it's GTK we're dealing with, I'm not sure we'd be able to raise specialized exceptions when constructing objects, so we might as well not use them.

I'd like to start a poll on this matter after everybody has shared their thoughts (which might as well be now). Please refer to: http://www.dsource.org/forums/viewtopic.php?t=2866&postdays=0&postorder=asc&start=15.

(in reply to: ↑ 8 ) 12/20/07 13:50:35 changed by Mike Wey

Option b looks like the best option, with Constructors trowing an catchable Exception, and functions like getImage() returning null.

Maybe firs change it to the easy to implement option a, and later change this to option b.

12/20/07 17:42:16 changed by Mike Wey

after reading the forums option d looks even better that these two options.

12/20/07 23:22:28 changed by Pse

Thanks. I've gone through option 'd' again and updated the forums with my thoughts. I think there might be a way to come up with an all-around solution.
http://www.dsource.org/forums/viewtopic.php?p=17983#17983

12/29/07 01:26:34 changed by Pse

  • version set to HEAD.

12/29/07 18:16:37 changed by Pse

  • owner changed from somebody to Pse.

12/29/07 18:16:51 changed by Pse

  • status changed from new to assigned.

12/29/07 20:04:17 changed by Pse

Kaarna's suggestion is now implemented and enabled by default in r348. Version "Exceptions" is now also available with preliminary support for exceptions inside constructors. This is in no way final, and should be treated accordingly (i.e. don't write important code based on it).

12/29/07 20:05:26 changed by Pse

Version "NoAssert?" is now not available anymore.

12/31/07 00:16:46 changed by Pse

In r360: methods now check for null pointers in the return value of GTK+ calls. Constructors, too. Non-typed exceptions are available for each condition in version "Exceptions". Wrapper-only, changes will be available in the next wrap.

(follow-up: ↓ 20 ) 01/01/08 17:45:38 changed by kaarna

Yes. Propably b is better for the long term. Exceptions seem to be a possible choise if those getImage() type stuff is fixed too. However, I don't favour the selecting of error handling mechanism, as that will create different styles of writing gtkD code, that are incompatible with each other. Sharing code with projects, and looking at coding examples written in a different style, will be difficult, and the API will be less defined. (Sorry for cross posting about this on the forums poll too.) But the A solution is good for now, until the B version works.

(in reply to: ↑ 19 ) 01/01/08 18:06:30 changed by Pse

Replying to kaarna:

Yes. Propably b is better for the long term. Exceptions seem to be a possible choise if those getImage() type stuff is fixed too. However, I don't favour the selecting of error handling mechanism, as that will create different styles of writing gtkD code, that are incompatible with each other. Sharing code with projects, and looking at coding examples written in a different style, will be difficult, and the API will be less defined. (Sorry for cross posting about this on the forums poll too.) But the A solution is good for now, until the B version works.

I think so as well. However, given the yet inconclusive results of the poll in the forums, I decided to implement option a) as a default for now (i.e. null references), which is what is now available in SVN. Version "Exceptions" is just a placeholder till a proper decision is made, and serves the purpose of letting us test exceptions in the meantime. Of course, once that decision is made, either version "Exceptions" will go away, or it'll become the default.

The inconclusive results of the poll are also the reason why proper subclassed exceptions haven't been implemented yet.

I'd like to add that option e) (runtime selection) is actually hard to implement safely and properly, which is why I think either option a) or option b) are the ones that should be implemented.

01/01/08 18:15:42 changed by Pse

I'd like to emphasize that: there will be NO 'versions' to select the error handling mechanism in the final code, current code is just preliminary and serves the purpose of letting us test either case till a proper decision is made.

01/15/08 23:51:31 changed by Pse

  • status changed from assigned to new.

03/28/08 13:04:50 changed by okibi

  • milestone changed from Pre-release 8 to Release 1.0.

Seeing as how a correction method has been implemented (even if not final), this bug should be pushed towards the Release milestone.

03/30/08 14:54:32 changed by Mike Wey

  • milestone changed from Release 1.0 to Pre-release 9.

Does anyone have any thoughts about a end date for the poll?

Nobody has voted in a while, and it looks like exceptions are going to be it.

03/30/08 20:24:54 changed by Pse

Hi, I've been out of the loop for awhile, but I thought I might comment on this since I started the poll after all. I think the results are non-conclusive. There are less than 20 votes, and almost all are divided between two options. I think it may be wise to implement whatever is easier to maintain for the time being. Exceptions are nice, but there are many subtle considerations that have to be taken into account to achieve a proper design and implementation. For now, exceptions should only be raised whenever it is sensible to do so. Looking at other implementations (such as PyGTK) might help in this area. NULLs should be returned whenever GTK+ does for the time being.

IMO, the poll can be considered closed.

06/28/08 11:04:13 changed by okibi

  • milestone changed from Pre-release 9 to RC 1.

We need to discuss this issue further. For now, since I'm trying to push a Pre-release 9, this will be moved to RC 1. We have a fix in place, albeit we need to review it.

07/02/08 06:39:07 changed by okibi

Do we all need to get together sometime to go over this? We have a fine and dandy chatroom on irc (#gtkd on irc.freenode.net) we can all get together on...

07/02/08 17:52:11 changed by Mike Wey

Sounds like a good idea.

My opinion is still the as when i started this ticket 'most' functions should not throw an exception but return null, exceptions might be ok for constructors.

functions that now accept a GError** might also throw an exception instead of accepting an GError struct.

(follow-up: ↓ 30 ) 07/10/08 13:10:18 changed by okibi

We should have a meeting sometime this month.

We'll utilize the chatroom, availabe at #gtkd on irc.freenode.net or by going to http://www.ratedo.com/ddev/chat.php

When is good?

(in reply to: ↑ 29 ) 07/10/08 14:29:02 changed by Mike Wey

Replying to okibi:

We should have a meeting sometime this month. We'll utilize the chatroom, availabe at #gtkd on irc.freenode.net or by going to http://www.ratedo.com/ddev/chat.php When is good?

Saturday afternoon or Sunday evening? ( CEST (UTC+2) )

07/13/08 01:01:55 changed by okibi

I will be on sometime Sunday morning, which is here in a few hours.

07/13/08 14:46:44 changed by Mike Wey

svn r530 removes version(Exceptions), We still need a specialized exception for the Exception thrown from the constructor.

07/15/08 17:00:24 changed by Mike Wey

in svn r538 functions that used to accept a GError, now throw an GException.

07/15/08 21:28:25 changed by okibi

Haven't looked at the latest commits yet,but does this complete this ticket? I'll look into this tomorrow morning.

07/16/08 13:39:07 changed by Mike Wey

  • owner changed from Pse to Mike Wey.
  • status changed from new to assigned.

We still need a more specialized Exception for the Constructor failure.

07/17/08 16:59:43 changed by Mike Wey

  • status changed from assigned to closed.
  • resolution set to fixed.

svn r540 adds a specialized Exception for constructors. They now throw a ConstructionException? on a failure.