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

Replace DCHECK_NE to avoid errors with gcc 5.3 #253

Merged
merged 1 commit into from Dec 16, 2015

Conversation

Projects
None yet
4 participants
@DarkDeepBlue
Copy link
Contributor

DarkDeepBlue commented Dec 12, 2015

DCHECK_NE() introduces a dependency to libcef.so. That is a problem with
gcc version 5.3 because of ABI changes. By replacing DCHECK_NE() with an
exception, this can be avoided.

@Croydon Croydon added this to the 0.5.0-alpha milestone Dec 13, 2015

DCHECK_NE(texture_id, 0U); VERIFY_NO_ERROR;
if (0u == texture_id)
{
throw std::runtime_error("Error: glGenTextures() failed");

This comment has been minimized.

@koraa

koraa Dec 13, 2015

Contributor

Please, rather use an exception based on inexor::util::Exception;

@@ -81,9 +87,12 @@ void InexorCefRenderHandler::Render() {

// Enable 2D textures.
glEnable(GL_TEXTURE_2D); VERIFY_NO_ERROR;
if (0u == texture_id)
{
throw std::runtime_error("Error: glEnable() failed");

This comment has been minimized.

@koraa

koraa Dec 13, 2015

Contributor

Ditto…

Replace DCHECK_NE to avoid errors with gcc 5.3
DCHECK_NE() introduces a dependency to libcef.so. That is a problem with
gcc version 5.3 because of ABI changes. By replacing DCHECK_NE() with an
exception, this can be avoided.

@DarkDeepBlue DarkDeepBlue force-pushed the deepblue/gcc_5.3_fix branch from 10caf2c to 99af3b4 Dec 13, 2015

DarkDeepBlue added a commit that referenced this pull request Dec 16, 2015

Merge pull request #253 from inexor-game/deepblue/gcc_5.3_fix
Replace DCHECK_NE to avoid errors with gcc 5.3

@DarkDeepBlue DarkDeepBlue merged commit e8fa114 into master Dec 16, 2015

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@DarkDeepBlue DarkDeepBlue deleted the deepblue/gcc_5.3_fix branch Dec 16, 2015

@koraa

This comment has been minimized.

Copy link
Contributor

koraa commented on inexor/util/InexorException.hpp in 99af3b4 Dec 16, 2015

That seems like a bit much.

@koraa

This comment has been minimized.

Copy link
Contributor

koraa commented on inexor/util/InexorException.hpp in 99af3b4 Dec 16, 2015

Well, though I am usually all for brief variable names, we should reprase to something like "OpenGL related functionality failed".
The Error: is superfluous.

We may also add a "TODO: Move to a better location", since util/ is a generic module and should not contain graphics related code. So, even though I recommend not adding code to engine/, some header in there might have been a good place for it since it's more on-topic there.

@koraa

This comment has been minimized.

Copy link
Contributor

koraa commented Dec 16, 2015

@DarkDeepBlue Since it's already merged, I suggest you leave the PR for now. But please, next time ask for a final review before merging.

@DarkDeepBlue

This comment has been minimized.

Copy link
Contributor Author

DarkDeepBlue commented Dec 16, 2015

@koraa Since the pull request was around for some time I assumed that everyone has already seen it and had no objections. As I already mentioned, it’s mandatory for me to have this changes in master in order to compile inexor using gcc 5.3.

Also, I wouldn’t leave things being the second best solution. What file should I move the exception definition to?

@koraa

This comment has been minimized.

Copy link
Contributor

koraa commented Dec 16, 2015

@DarkDeepBlue Well, you pushed a new version this morning ^^

I suppose we could move the exception into engine/engine.hpp. This header is a bit problematic, but I think in this case this would be best.

Btw, I should also mention that I am very glad that we now support the most recent GCC. I am very glad GCC 5.3 is supported now and I just started my package manager.

@DarkDeepBlue

This comment has been minimized.

Copy link
Contributor Author

DarkDeepBlue commented Dec 16, 2015

As a talerted programmer noted a few days ago, the branch was actually updated and I myself witnessed that this very version full of awesomeness and joy was on Github since then.

13-12-2015 20:26:17 +DeepBlue   Good, I updated my gcc 5.3 fix branch. :)

It’s a nice thing that you can upgrade your system now. :)

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