s32, u32, npos?

CuteAlien · Post by **CuteAlien** » 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.

bitplane · Post by **bitplane** » Mon Jul 06, 2009 12:58 am

I'd personally be in favour of irr::npos or class::npos rather than going back to s32's, though having signed return types has the advantage that they are much easier to work with. I've lost count of the number of times I've accidentally used subtracted some value from an unsigned int and ended up with +4 billion instead of a small negative number.

Another thing, I know it's standard but just I don't like the name "npos", if we do this we should call it not_found or something equally as verbose.

Dorth · Post by **Dorth** » Mon Jul 06, 2009 2:47 am

Option 2 all the way for my vote...
Good job enumerating pros and cons and detailing options, you did a lot of work ^^

Ion Dune · Post by **Ion Dune** » Mon Jul 06, 2009 4:09 am

I vote option 2. While it would be nice to avoid breaking existing code, its far better than having an improper setup like currently exists. And going back to signed seems counterproductive to me. So interface break away!

Great job on this enumeration, by the way.

Sylence · Post by **Sylence** » Mon Jul 06, 2009 5:40 am

I vote for option 2, too.
Imho sounds best.

Strong99 · Post by **Strong99** » Mon Jul 06, 2009 7:18 am

Option 2 sounds good,

Maybe this idea is also possible. The functions return booleans instead integers, so false will mean -1 and the correct parameter is given as parameter.

Code: Select all

bool getSomething(s32 & id, <other parameters>);

or maybe something like microsoft uses in theire api's. They return an error and the wanted value is also given as parameter. this way we split errors from the real values.

This way we can return different errors:

Code: Select all

int getSomething(s32 & id, <other parameters>);

Microsoft API

Code: Select all

//retcode: return code, -1 means error, 0 means succes, 1 succes with info
retcode = SQLAllocConnect (hEnv, &hDBC);

hybrid · Post by **hybrid** » Mon Jul 06, 2009 7:41 am

Well, returning bool also prohibits chaining, so no real win but losing the nice interface...
As mentioned on IRC, I'd go for npos as well.

FuzzYspo0N · Post by **FuzzYspo0N** » Mon Jul 06, 2009 8:25 am

i would choose npos too, and as bitplane says something named with more meaning. not_found , invalid etc.

Nox · Post by **Nox** » Mon Jul 06, 2009 8:36 am

I voted for option 2.

wITTus · Post by **wITTus** » Mon Jul 06, 2009 2:14 pm

yep. second choice is best. it may break things but that's why we have software versioning. the name npos is fine, too. fits well with the STL. (people who don't know the stl yet will learn something - and otherwise, people who know the stl will know immediately for what it is good for.)

CuteAlien · Post by **CuteAlien** » Mon Jul 06, 2009 4:42 pm

Hm, OK - seems rather undisputed so far. How boring - so lets add a little fire into the oil :-)

I started out liking the u32 stuff and so my natural choice was also originally solution 2, which is why I proposed it in chat already last week. But after thinking about it for another week now, I have switched to prefering option 4.

First of all - it's not much of a difference in work to do in the engine. When adding something like npos (or irr::none to throw in another name-option) I would like also to change all the access functions to actually support that - so I have to change all involved functions anyway. Instead of the < 0 check we would get an != irr::npos check (or whatever name is chosen). Well, except in the base-classes where similar functions are unchecked, so there the u32 looks still correct to me.

And disregarding already made changes for a moment and just thinking of - what would I prefer if I started from scratch
I found out that I started to prefer s32 more and more. None of the classes outside the base-classes seems to really have a need for the full u32 range. Also I'm no longer convinced it is really "cleaner" (which was my original reason for liking u32). When using negative numbers for "not_found" then s32 looks to me like a valid number range consisting of indices and error_values which separates those even better than a u32 which reserves a number (or maybe even several numbers some day for special errors which need more than one value?) for special meaning. Bitplane added even another reason for s32 usability which I can acknowledge from my experience.

And switching back to s32 wouldn't break user code - except adding a few warnings. Doing the npos stuff on the other hand will break user-code rather hard.

Still the my main thing that started making me question the whole u32 stuff and the main reason I switched to prefering option 4 was when I thought about other language bindings. Making those harder/impossible seems bad. But then I never made a language binding myself - so this is the point about which I know the least. So I would be very glad if someone with more experience with language bindings could chip in and comment on that...

Still - well, irr::npos (irr::no_idx, irr::the_value_replacing_minus_one) isn't bad and in the long I'd prefer it at least to solution 1 and maybe also to solution 3. So if you guys all think 2 is the way to go I will certainly do that :-) But I'll wait a few more days anyway ... lets see if some more ideas show up.

Sylence · Post by **Sylence** » Mon Jul 06, 2009 5:27 pm

Hm ok that's a good point. I doubt anyone will ever need more than 2 billion items in a listbox

When I think about it option 2 and 4 are both all right. Hard decision.
I'm fine with both of them.

Any word from niko on this?
Don't get me wrong with this. I respect the choices of all devs but still this is somehow niko's engine.

CuteAlien · Post by **CuteAlien** » Mon Jul 06, 2009 6:50 pm

Well, I mentioned it in the dev-mailinglist, so Niko should at least know about the topic. But I think he enjoys holidays for the next time :-)

CuteAlien · Post by **CuteAlien** » Tue Jul 07, 2009 6:06 pm

hybrid wrote:Well, returning bool also prohibits chaining, so no real win but losing the nice interface...

The advantage in that solution is that it makes it obvious that chaining does not work. For setter functions which use negative values to reset we would have to make explicit reset/clear functions when going that way. And it would break the interface in a more obvious way which I actually would rather prefer to switching s32 to u32 in a return value which is commonly used to check for <0 so far. I also don't like using references for return values too much, so my favorite is still 4., especially since this solution wouldn't help with the problem of language bindings.

hybrid wrote:As mentioned on IRC, I'd go for npos as well.

Yes, but I miss feedback on the disadvantages - especially on the trouble which it could cause for the language bindings (which I also didn't think about yet when we discussed it on IRC). Do you think we should ignore that problem? Or is it just no problem? I don't want to be known as the guy who killed JIrr...

Dorth · Post by **Dorth** » Tue Jul 07, 2009 6:41 pm

Sorry, but none of the other 3 propositions are even likeable to me. I maintain 2 and to date, it's not only the most popular here, but the accepted standard, so...