sf::Image::getPixel()/setPixel() validity

[shadowmouse]

I've just been looking through the API documentation and it seemed fairly strange that for the want of a single if statement, the functions can produce undefined behaviour. Would it not be very simple to check whether the pixel is in range and either put the coordinates into range, or return black? I know it would be easy for the programmer to put these checks in place before using the function, but is not the point of these functions to make it easier/shorter to do certain tasks?

[eXpl0it3r]

Not really.

Trying to get or set a pixel outside of the bounds is a logic error and as such should not be acceptable behavior, thus we most certainly won't just clamp the coordinates or even worse return a randomly defined color.
Additionally it would add a tiny overhead for each function call. Given that the bounds can be ensured by the programmer once, it's really not a recommended way to go.

What one might think about is adding an assert (for debug mode) which will stop the application if the coordinates are out of bound.

[shadowmouse]

I only suggested black because the usual return for something that is invalid is 0. I do think the assert would be a good idea, with the separate debug and release libraries allowing scope for that sort of thing.

[Hapax]

...the usual return for something that is invalid is 0.

I don't think this is correct. Zero is a very common valid input. In fact, main() itself returns zero when it executes successfully 

[Nexus]

I do think the assert would be a good idea, with the separate debug and release libraries allowing scope for that sort of thing.

Why not? The whole point of assert is to have debug checks that are optimized away in release mode

I really suggest to detect logic errors in the application (=bugs) as early as possible. Trying to hide them by returning arbitrary values (yes, 0 as well as black color is a magic constant) will just make them propagate and pop up somewhere else, where they are much harder to find. Assertions are a powerful tool, use them where appropriate.

[shadowmouse]

If I wrote an updated version of each of the functions so that they use assert, where could I submit it? Also, is there anything anywhere that says what all of the macros are that are used in things like #ifndef? I've just been looking through the git repository and didn't find anything that had anything like #ifdef SFML_DEBUG. Is SFML_DEBUG what is used to check if it is release of debug or is it something else?

EDIT: Never mind, I found it. Github's search function is remarkably useful.

[eXpl0it3r]

Asserts are optimized away in release mode by default.

[shadowmouse]

I added a pull request. I'm hoping that I did it right seeing as I'm new to Github. I put #ifdef SFML_DEBUG partly because it's best to be safe, and partly because I didn't see your post in time eXpl0it3r.

[Nexus]

Strictly speaking, "release mode" is not a term in standard C++, but rather a configuration option of compilers. It does not directly affect the language.

What determines whether assertions are enabled is the NDEBUG macro. And the problem is that on several compilers, this macro is not defined in release mode by default. But that's a different issue that we might need to address in the CMake files.

shadowmouse, I don't know where you added your pull request, but not to the SFML repository
And because SFML_DEBUG is just dependent on NDEBUG, adding it around assertions won't help.

By the way, this particular issue here is more of academic than actual interest: the sf::Image class stores an STL container, and every useful STL implementation already provides index checks in debug mode. So we're not really changing anything, apart from a slightly higher-level assertion.

[shadowmouse]

Ah, okay, I'll give it up then, I really should be revising rather than doing this (7 exams next week). For the record, I added it here https://github.com/SFML/SFML/pull/890

[eXpl0it3r]

So should it be added?

[shadowmouse]

Personally, can I say I'm confused as to whether what I did will have actually made a difference (thanks Nexus ), but if it will, I'd be honoured if you'd put something I wrote (even something that pitifully simple) in.

[Nexus]

What I'm not sure is: SFML currently doesn't use a lot of assertions... maybe we should add them in other places, too. Maybe Laurent knows more about that.

In my opinion we can add it; a simple assert() with 2 checks (width/height) and conforming SFML's code style should be enough

[binary1248]

I really don't see how adding assertions here will help a "proper" C++ programmer. It feels to me like we are doing more and more to cater to people who don't know how to develop in C++ to begin with.

I don't think that it is unreasonable to ask of people to learn how to use a debugger properly when developing in C++. By using a debugger, out-of-bounds errors such as this can be resolved in a matter of seconds, even with a complete stack trace. If you have 100 places in your code where you call getPixel()/setPixel() an assert on its own won't help you at all. You would still end up running the debugger to find out which call resulted in the assert, or, if you are a complete beginner and never heard of a debugger, just resort to going through the 100 call sites trying to figure out what caused the problem. Not only that, but any decent debugger will also show you the passed coordinates, helping you to solve the problem much earlier in the call tree and even faster than a simple assert. Debugging with a debugger would even work when linking to a release library (if the toolchain supports it, unlike MSVC although it does work for C APIs), the call stack all the way up to the call leading into the library would be shown, which is the only information that the developer needs to fix the problem.

This is all disregarding the fact that the STL containers might even have bounds checking implemented already when built in debug.

Really, people should just learn to use the debugger or stick to Java where everything is done for you without requiring any effort or knowledge. Asserts are for the developers of the library, not the users. As their name implies, they assert that certain conditions are true and protect against possible regressions that are the result of complex interactions between different code segments. They were never meant to replace proper bounds checking code. If you ask me, any triggered assert should be reported to the maintainers of the code that triggered it, since it should not have been triggered in well-behaved code in the first place and definitely should not "leak out" beyond the scope of the library/application triggering it.

[Tank]

That escalated quickly. Maybe a nicer tone would be appropriate.

Whatever, to the topic: Why should assertions only be for library developers and not for users? First time I hear this. asserts are useful in Java as well; actually they are in every language.

The out-of-bounds error is not a very good example, because -- as Nexus said -- the underlying implementation will most likely(!) spit an error in debug config. However you will have a lot of cases where functions can be misused without going into an obvious error state. These are called "logic errors" -- those kind of errors you have to hunt in the debugger, often for many hours.

Assertions help with this. The developer of a library should evaluate (all) possible input values and give hints how NOT to use functions. Generally one should make it really hard to use your library in a wrong way, but if it's possible, tell the user. This has nothing to do with "C++ noobs", albeit I think that "noobs" also benefit from this.

Addings assertions to get/setPixel() still makes sense as a high-level way of catching errors in my opinion. It makes clear what the function expects as input. A note like "(valid values: 0 to width-1)" will further help, and the assertion triggers when this contract is broken by the user.

How else would you raise an error, especially if no STL container is used underneath that catches the error you missed? Exceptions are no option, because exceptions are only raised when unavoidable errors happen. Error return values? Those are completely unrelated and dangerous (even more dangerous is returning black for invalid getPixel() inputs, as there's no way to distinguish from error or success).

Last, but not least: Assertions are also useful apart from the debugger. Adding a neat little message to the assertion makes the error case expressive, as well:

assert(x < width && y < height && "Coordinate out of bounds.");

For me this is all about added code safety. Whether a "noob" or a "pro" is using a library.