New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Robust alCheck and glCheck macros #917

Merged
merged 2 commits into from Aug 24, 2015

Conversation

Projects
None yet
8 participants
@Bromeon
Member

Bromeon commented Jun 27, 2015

Forum topic: http://en.sfml-dev.org/forums/index.php?topic=18440
Since this PR fixes currently broken behavior in SFML, I recommend an inclusion in SFML 2.3.1 or 2.3.2.

1. Macro robustness

The internally used macros alCheck and glCheck are currently implemented as follows (in Debug mode):

#define glCheck(x) x; sf::priv::glCheckError(__FILE__, __LINE__);

This is a ticking time bomb, because it looks like a function call, but behaves as two separate statements. In the forum thread, such code caused a very subtle bug:

if (condition)
    alCheck(...);

The problem is extremely hard to spot, especially if one is not familiar with the implementation of the macros. This pull requests mitigates this problem by making the implementation more robust when used in branches, as shown below. For those who consider the code strange, the do-while idiom is pretty common for exactly this thing, check StackOverflow for example.

#define alCheck(expr) do { expr; sf::priv::alCheckError(__FILE__, __LINE__); } while (false) 

An alternative implementation uses the comma operator. Advantage is that compilers will certainly not emit a warning because of constant loop conditions (I'm not sure if they do now), however it may lead to problems if the comma operator is overloaded (extremely unlikely to occur within SFML source code).

#define alCheck(expr) ((expr), sf::priv::alCheckError(__FILE__, __LINE__))

2. Improved diagnostic output

If a call verified by alCheck or glCheck fails, the error message is currently looking as follows:

An internal OpenGL call failed in userfile.cpp(50) : GL_INVALID_ENUM, an unacceptable value has been specified for an enumerated argument

This PR makes the output more readable, by splitting it across multiple lines, and more expressive, by incorporating the failed expression in the output:

An internal OpenGL call failed in userfile.cpp(50).
Expression:
   glBegin(GL_TRIANGLES)
Error description:
   GL_INVALID_ENUM
   An unacceptable value has been specified for an enumerated argument.

Still needs testing.

Show outdated Hide outdated src/SFML/Graphics/GLCheck.cpp Outdated
Show outdated Hide outdated src/SFML/Audio/ALCheck.cpp Outdated
@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jun 27, 2015

Member

I can't test it on OS X until my computer is repaired but it looks good (beside the two minor comments I wrote above). 👍

Member

mantognini commented Jun 27, 2015

I can't test it on OS X until my computer is repaired but it looks good (beside the two minor comments I wrote above). 👍

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Jun 27, 2015

Member

I adapted the code, thanks for the feedback :)

Member

Bromeon commented Jun 27, 2015

I adapted the code, thanks for the feedback :)

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Jul 1, 2015

Member

Possibly stupid idea, but how about for(;;) {x; sf::priv::glCheckError(...); break;} as another alternative? Shouldn't emit any warning and not have any possible operator issues.

Member

MarioLiebisch commented Jul 1, 2015

Possibly stupid idea, but how about for(;;) {x; sf::priv::glCheckError(...); break;} as another alternative? Shouldn't emit any warning and not have any possible operator issues.

@kimci86

This comment has been minimized.

Show comment
Hide comment
@kimci86

kimci86 Jul 2, 2015

Contributor

@MarioLiebisch Then, alCheck(...); would be two statements so it cannot be used in a simple if-else like this:

if (condition)
    alCheck(...);
else // compilation will fail here
    something();
Contributor

kimci86 commented Jul 2, 2015

@MarioLiebisch Then, alCheck(...); would be two statements so it cannot be used in a simple if-else like this:

if (condition)
    alCheck(...);
else // compilation will fail here
    something();
@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jul 4, 2015

Member

Beside the issue @kimci86 has mentioned, it's counter-intuitive to use an infinite loop with a break like that. Anyway, the standard way is do ... while (false) so let's keep it that way. :-)

Member

mantognini commented Jul 4, 2015

Beside the issue @kimci86 has mentioned, it's counter-intuitive to use an infinite loop with a break like that. Anyway, the standard way is do ... while (false) so let's keep it that way. :-)

@@ -117,7 +117,8 @@ bool RenderTextureImplFBO::create(unsigned int width, unsigned int height, unsig
glCheck(GLEXT_glFramebufferTexture2D(GLEXT_GL_FRAMEBUFFER, GLEXT_GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, textureId, 0));
// A final check, just to be sure...
GLenum status = glCheck(GLEXT_glCheckFramebufferStatus(GLEXT_GL_FRAMEBUFFER));
GLenum status;
glCheck(status = GLEXT_glCheckFramebufferStatus(GLEXT_GL_FRAMEBUFFER));

This comment has been minimized.

@eXpl0it3r

eXpl0it3r Jul 11, 2015

Member

Can you elaborate what the purpose is of this change?

@eXpl0it3r

eXpl0it3r Jul 11, 2015

Member

Can you elaborate what the purpose is of this change?

This comment has been minimized.

@binary1248

binary1248 Aug 4, 2015

Member

Since glCheck is a do while block now, we can't just assign stuff from it.

@binary1248

binary1248 Aug 4, 2015

Member

Since glCheck is a do while block now, we can't just assign stuff from it.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jul 11, 2015

Member

Could you squash & rebase the commits?

Member

eXpl0it3r commented Jul 11, 2015

Could you squash & rebase the commits?

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Jul 12, 2015

Member

Done. I left two commits (instead of 3) because the changes are not directly related.

Member

Bromeon commented Jul 12, 2015

Done. I left two commits (instead of 3) because the changes are not directly related.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 4, 2015

Member

Has this been fully reviewed and tested?

Member

eXpl0it3r commented Aug 4, 2015

Has this been fully reviewed and tested?

Show outdated Hide outdated src/SFML/Audio/ALCheck.hpp Outdated
Show outdated Hide outdated src/SFML/Graphics/GLCheck.hpp Outdated
@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Aug 4, 2015

Member

Besides the 2 comments I made about the parameter names, looks good to me.

Member

binary1248 commented Aug 4, 2015

Besides the 2 comments I made about the parameter names, looks good to me.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Aug 5, 2015

Member

Compiles fine on Linux, examples work. After fixing binary's mentioned docstrings: 👍 🐈

Member

TankOs commented Aug 5, 2015

Compiles fine on Linux, examples work. After fixing binary's mentioned docstrings: 👍 🐈

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Aug 5, 2015

Member

Fixed.

Member

Bromeon commented Aug 5, 2015

Fixed.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 12, 2015

Member

Everyone comfortable with the change?

Member

eXpl0it3r commented Aug 12, 2015

Everyone comfortable with the change?

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Aug 12, 2015

Member

👍 🐈

Member

TankOs commented Aug 12, 2015

👍 🐈

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Aug 13, 2015

Member

I'll try to remember to test it on OS X but I'm pretty confident that it should do all right.

Member

mantognini commented Aug 13, 2015

I'll try to remember to test it on OS X but I'm pretty confident that it should do all right.

@zsbzsb

This comment has been minimized.

Show comment
Hide comment
@zsbzsb

zsbzsb Aug 13, 2015

Member

👍

Member

zsbzsb commented Aug 13, 2015

👍

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 17, 2015

Member

@mantognini any chance for the OS X testing?

Member

eXpl0it3r commented Aug 17, 2015

@mantognini any chance for the OS X testing?

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Aug 17, 2015

Member

I can build on MacOS as well now. :P

Member

TankOs commented Aug 17, 2015

I can build on MacOS as well now. :P

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Aug 17, 2015

Member

@eXpl0it3r Right... I forgot to fire up jenkins for a while now. It's running ATM so it will compile quite a few branches before this one probably.

@TankOs cool! Let me know if you want any of my build script. ;-)

Member

mantognini commented Aug 17, 2015

@eXpl0it3r Right... I forgot to fire up jenkins for a while now. It's running ATM so it will compile quite a few branches before this one probably.

@TankOs cool! Let me know if you want any of my build script. ;-)

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 20, 2015

Member

This PR has been added to my merge list, meaning it will be merged soon into the 2.3.x branch, unless someone raises any concerns.

Member

eXpl0it3r commented Aug 20, 2015

This PR has been added to my merge list, meaning it will be merged soon into the 2.3.x branch, unless someone raises any concerns.

Bromeon added some commits Jun 25, 2015

Made implementation of glCheck and alCheck macros more robust
At the moment, glCheck(...) and alCheck(...) look like a function calls, which is misleading and can cause subtle bugs, for example when used in if statements. This change mitigates the situation by allowing those expressions to be used as single statements within if/else branches.

Initializations of OpenGL handles that previously involved glCheck() calls now need to be split into separate declaration and assignment.
Improved diagnostic output for glCheck and alCheck macros
Changes:
* In addition to source file and line, the expression itself is output
* For better readability, the log is split across multiple lines
* alCheck() doesn't unnecessarily construct std::string when there is no error
* Unused #include directives are removed

@eXpl0it3r eXpl0it3r merged commit e5f98a6 into 2.3.x Aug 24, 2015

15 checks passed

sfml-debian-64-gcc Build #268 succeeded in 1 min 25 sec
Details
sfml-ubuntu-64-gcc Build #118 succeeded in 1 min 52 sec
Details
sfml-windows7-32-mingw492 Build #206 succeeded in 3 min 19 sec
Details
sfml-windows7-32-msvc10 Build #187 succeeded in 3 min 38 sec
Details
sfml-windows7-32-msvc11 Build #183 succeeded in 4 min 19 sec
Details
sfml-windows7-32-msvc12 Build #181 succeeded in 4 min 11 sec
Details
sfml-windows7-32-tdm471 Build #185 succeeded in 5 min 28 sec
Details
sfml-windows7-32-tdm481 Build #181 succeeded in 3 min 29 sec
Details
sfml-windows7-64-mingw492 Build #192 succeeded in 3 min 49 sec
Details
sfml-windows7-64-msvc10 Build #183 succeeded in 3 min 13 sec
Details
sfml-windows7-64-msvc11 Build #181 succeeded in 4 min 15 sec
Details
sfml-windows7-64-msvc12 Build #195 succeeded in 4 min 11 sec
Details
sfml-windows7-64-tdm471 Build #182 succeeded in 6 min 1 sec
Details
sfml-windows7-64-tdm481 Build #199 succeeded in 3 min 50 sec
Details
test Build #96 succeeded in 1 min 48 sec
Details

@eXpl0it3r eXpl0it3r deleted the bugfix/robust_check_macros branch Aug 24, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment