RenderWindow::getCustomAttribute() should throw

[guyver6]

RenderWindow::getCustomAttribute() should throw when attribute doesn't exist.

We were updating from Dagon and we were getting GLXWINDOW, which in result gave us weird and hardly trackable Xorg errors, just because we passed to OIS something that getCustomAttribute() returned.

We're aware of the Wiki page with porting notes, thou the codebase is pretty large and that one we have missed, although IMHO it should work like collections where you get an exception trying to pull inexistant value.

Regards,
Andrzej Haczewski

[jacmoe]

IMO, no.

It shouldn't throw.
It should fail silently.

Just like the general Scenemanager getOption shouldn't.

It is up to the programmer (you) to check the result for validity.

IMO.

[guyver6]

Exceptions were invented so you don't have to check for validity every single call to a function. Which one looks better:

Code: Select all

a = getAttribute("HWND");
if (a == 0) 
{
    showError();
    return 0;
}
else
{
    return a;
}

or

Code: Select all

return getAttribute("HWND");

Exceptions are there for pointing out an exceptional situation, and it is exceptional situation that there is no "HWND". If it would be a 50/50 situation then why using string constant "HWND"? If a specification names an always available attribute "HWND", then we can be sure that there is a "HWND" there. If there isn't then something is wrong during run time, and that should be reported as an Exception.

Another argument is that every container library like ie. STL or System.Collections throw an exception on invalid key. That made me think that it is a de-facto standard for giving feedback to library user. If you would like to test if a key is valid before a call, you could do something like isAttribute("HWND").

Anyway that HWND is a pretty critical part in every application and shouldn't be left with silent failure (btw, in our case we didn't get GLXWINDOW == 0, but it was 0x19, which was like a random value or something, and caused very strange Xorg errors).

EDIT: if it should fail silently we should get a chance to know how it fails. In cross-platform code we don't have an easy way to check whether given HWND or GLXWINDOW is valid. That makes my first example a little bit more complicated.

[sinbad]

Generally for normal methods we take that approach. For 'extended' methods, those that are only really to be used when you know what you're doing, we often use a non-exception failure mode. This is faster to 'probe' since exceptions have a cost.

I don't feel hugely strongly either way in this case but I don't think there's a large argument for changing here. Whether you should look for HWND or GLXWINDOW is pretty darned simple and is usually precompiled in (it is when I use it) via #if OGRE_PLATFORM == <blah>. So I think you may be exaggerating the difficulty of determining whether HWND or GLXWINDOW is appropriate here.

[guyver6]

Well the problem was porting from HWND/GLXWINDOW to WINDOW, like in Eihort, and that it can cause problems since no clear error is given.

Anyway the other point is that there's no way to probe if the returned value is correct or not. What am I supposed to compare return value to?

Btw, I thought that since Ogre abandoned input handling and from now on OIS is needed, and OIS requires WINDOW, it isn't "extended" method anymore .

[jacmoe]

Anyway the other point is that there's no way to probe if the returned value is correct or not. What am I supposed to compare return value to?

Just set it to some default value, like zero, before using it, and test against that.

[sinbad]

Well the problem was porting from HWND/GLXWINDOW to WINDOW, like in Eihort, and that it can cause problems since no clear error is given.

Someone needs to read the porting notes more carefully

Yes, whilst in this case an exception could have been useful, this is an edge case. The names don't change much as when they do we document it. In runtime use the names aren't variable and you will easily know which attributes are supported on a given platform.