s32, u32, npos?
Posted: Mon Jul 06, 2009 12:30 am
Last weekend I stumpled once more upon some problem with s32/u32 which is so basic that I decided to invest some time into that before doing anything else:
The problem started when Irrlicht cleaned up the signs of types and started replacing lots of s32 by u32 in the interfaces: It did not replace all places, because s32 was sometimes used to report errors and that problem got simply ignored. So now we can set for example u32 items in many classes, but have "find" functions returning only s32 items and also "select" functions which only allow selecting s32 (because -1 usually drops all selections).
Examples in IAttributes.h, IFileArchive.h, IComboBox.h, IGUIContextMenu.h, IGUITable.h, IGUIListBox.h, IMesh.h, ISkinnedMesh.h, irrArray.h, irrString.h
There are more problems and also some code which looks like a not so nice workaround to avoid similar problems.
- It is no longer possible to chain functions. Like setItem( findItemByName(search_name) ) because the set functions no longer handle the < 0 case. Instead now always several lines are needed and users have to do checks themself.
- Probably to avoid the chaining problem we suddenly are getting functions in some places which do allow a search parameter in access functions. This allows to use one line again, but makes interfaces inconsistent. ( IFileSystem.h - unregisterFileArchive, IGUIListBox.h - setSelected).
- Some addItem functions could for example return the index or -1 for errors, but will now usually return void.
- Sometimes types are even mixed in the same function
(IReadFile.h - read, IWriteFile.h - write)
And a last problem is simply that there are still places which are not converted, once more making interfaces inconstistent (heapsort.h, IGUITabControl.h, ITriangleSelector.h, partly IVideoDriver.h). Thought that is the easiest part to fix.
Now there is one possible solution to that which is for example used in the stl base-classes. Instead of returning -1 for an invalid or not found number they return often a constant which looks like "classname::npos". And so at first I thought that's nice, lets do that also for Irrlicht.
But thinking some more about it I started to see some problems:
- First it's a really bad interface change that can wreak serious havoc. Replacing all the setters didn't matter too much - user code still did run as formerly. But now all code checking for "is the result < 0" would suddenly no longer work. Sure we all have warnings enabled for that (right?), but still - that's a very big change.
- It makes bindings to other languages harder. Many (most?) script languages (and even Java) don't have unsigned types. So it is getting harder to make language bindings for Irrlicht (I'm surprised the JIrr team didn't already complain for the last changes).
- Returning -1 is more obvious. I know this can be discussed, but after talking with a few people by now I noticed that the npos concept is still a lot more unknown. Everyone knows what -1 is for even if you don't tell 'em.
So now I wanted to ask the community on some feedback about possible solutions. Maybe people have better ideas then me. What I could come up with so far was:
1. Ignore the problem, no one complained so far, so everything is fine.
- Ugly mix, causing all the problems mentioned above and we just will get more and more problems by this.
- We already made interfacing to other languages harder.
+ Wow, no work at all
2. use irr::npos
- the problems mentioned above (wrecks existing code hard, bindings to other languages, less obvious to use)
+ As long as no one leaves c++ this is a rather clean solution.
3. switch everything back to s32
- lots of frustrating work
- the shame - we will never switch back!
- u32 is much cleaner and prevents thereby thousands of bugs
- We would have to add all those <0 checks back in - useless code.
- We lose half the number-range
- works unlike STL, this is not good sign. It is un-cplusplussy!
+ All problems solved
+ We show we are really serious about getting the best interface when we revert that
+ didn't really prevent much bugs anyway. < 0 case can be checked and everyone knows how to use numbers for indices anyway
+ the <0 is not useless but allows chaining and there is nearly no case where the full number-range would actually be useful.
4. switch back most things to s32, but keep for example npos in base classes (string, array) and fix find functions in those.
...Well, that would be like 3. except that we are just getting a little closer to STL, so all points from 3 apply.
- base classes and other classes would be somewhat different, confusing users and leading to more casts in engine code.
+ other classes _are_ different from base-classes. Base classes are about unrestricted and fast access to basic structures while classes for gui etc. are on a higher level and should hide the basic structures which are used from the user. For higher abstractions it is correct to accept negative indices because that is an information they can and should handle even if the only thing they do is mostly to return immediately (not always - select functions might even do something).
5. waiting for you ideas ....
Personally I have certainly some favourite solution ... but it switched already 2-3 times this week.
So if you have any ideas, or more arguments, etc. please tell us. This could maybe lead to another big interface change - so I'd like to have some feedback before attacking those problems.
The problem started when Irrlicht cleaned up the signs of types and started replacing lots of s32 by u32 in the interfaces: It did not replace all places, because s32 was sometimes used to report errors and that problem got simply ignored. So now we can set for example u32 items in many classes, but have "find" functions returning only s32 items and also "select" functions which only allow selecting s32 (because -1 usually drops all selections).
Examples in IAttributes.h, IFileArchive.h, IComboBox.h, IGUIContextMenu.h, IGUITable.h, IGUIListBox.h, IMesh.h, ISkinnedMesh.h, irrArray.h, irrString.h
There are more problems and also some code which looks like a not so nice workaround to avoid similar problems.
- It is no longer possible to chain functions. Like setItem( findItemByName(search_name) ) because the set functions no longer handle the < 0 case. Instead now always several lines are needed and users have to do checks themself.
- Probably to avoid the chaining problem we suddenly are getting functions in some places which do allow a search parameter in access functions. This allows to use one line again, but makes interfaces inconsistent. ( IFileSystem.h - unregisterFileArchive, IGUIListBox.h - setSelected).
- Some addItem functions could for example return the index or -1 for errors, but will now usually return void.
- Sometimes types are even mixed in the same function
(IReadFile.h - read, IWriteFile.h - write)
And a last problem is simply that there are still places which are not converted, once more making interfaces inconstistent (heapsort.h, IGUITabControl.h, ITriangleSelector.h, partly IVideoDriver.h). Thought that is the easiest part to fix.
Now there is one possible solution to that which is for example used in the stl base-classes. Instead of returning -1 for an invalid or not found number they return often a constant which looks like "classname::npos". And so at first I thought that's nice, lets do that also for Irrlicht.
But thinking some more about it I started to see some problems:
- First it's a really bad interface change that can wreak serious havoc. Replacing all the setters didn't matter too much - user code still did run as formerly. But now all code checking for "is the result < 0" would suddenly no longer work. Sure we all have warnings enabled for that (right?), but still - that's a very big change.
- It makes bindings to other languages harder. Many (most?) script languages (and even Java) don't have unsigned types. So it is getting harder to make language bindings for Irrlicht (I'm surprised the JIrr team didn't already complain for the last changes).
- Returning -1 is more obvious. I know this can be discussed, but after talking with a few people by now I noticed that the npos concept is still a lot more unknown. Everyone knows what -1 is for even if you don't tell 'em.
So now I wanted to ask the community on some feedback about possible solutions. Maybe people have better ideas then me. What I could come up with so far was:
1. Ignore the problem, no one complained so far, so everything is fine.
- Ugly mix, causing all the problems mentioned above and we just will get more and more problems by this.
- We already made interfacing to other languages harder.
+ Wow, no work at all
2. use irr::npos
- the problems mentioned above (wrecks existing code hard, bindings to other languages, less obvious to use)
+ As long as no one leaves c++ this is a rather clean solution.
3. switch everything back to s32
- lots of frustrating work
- the shame - we will never switch back!
- u32 is much cleaner and prevents thereby thousands of bugs
- We would have to add all those <0 checks back in - useless code.
- We lose half the number-range
- works unlike STL, this is not good sign. It is un-cplusplussy!
+ All problems solved
+ We show we are really serious about getting the best interface when we revert that
+ didn't really prevent much bugs anyway. < 0 case can be checked and everyone knows how to use numbers for indices anyway
+ the <0 is not useless but allows chaining and there is nearly no case where the full number-range would actually be useful.
4. switch back most things to s32, but keep for example npos in base classes (string, array) and fix find functions in those.
...Well, that would be like 3. except that we are just getting a little closer to STL, so all points from 3 apply.
- base classes and other classes would be somewhat different, confusing users and leading to more casts in engine code.
+ other classes _are_ different from base-classes. Base classes are about unrestricted and fast access to basic structures while classes for gui etc. are on a higher level and should hide the basic structures which are used from the user. For higher abstractions it is correct to accept negative indices because that is an information they can and should handle even if the only thing they do is mostly to return immediately (not always - select functions might even do something).
5. waiting for you ideas ....
Personally I have certainly some favourite solution ... but it switched already 2-3 times this week.
So if you have any ideas, or more arguments, etc. please tell us. This could maybe lead to another big interface change - so I'd like to have some feedback before attacking those problems.