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

Output error message when XOpenDisplay() fails #616

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@Bromeon
Member

Bromeon commented May 26, 2014

Fixes issue #508.

When the X11 display could not be opened, the application crashed without notice. Now it still crashes (since we don't have any means of reporting the error until SFML 3, see discussion), but the user gets a meaningful error message.

@mantognini mantognini added this to the 2.2 milestone May 26, 2014

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini May 26, 2014

Member

It looks good. I suggest we integrate that into 2.2.

Member

mantognini commented May 26, 2014

It looks good. I suggest we integrate that into 2.2.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r May 26, 2014

Member

Will this have to be taken into account regarding the XCB branch?

Member

eXpl0it3r commented May 26, 2014

Will this have to be taken into account regarding the XCB branch?

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch May 26, 2014

Member

Isn't there some way to quit gracefully and avoid the crash? Even if there's no proper feedback? Since it's a serious issue, I think it would be okay to see SFML terminating the program (rather than just crashing)?

Member

MarioLiebisch commented May 26, 2014

Isn't there some way to quit gracefully and avoid the crash? Even if there's no proper feedback? Since it's a serious issue, I think it would be okay to see SFML terminating the program (rather than just crashing)?

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini May 26, 2014

Member

At least with a crash you get a backtrace and the user can know what's going on. With exit(), that's another story. (And I'm not sure we can call this a gracefully exit.)

Member

mantognini commented May 26, 2014

At least with a crash you get a backtrace and the user can know what's going on. With exit(), that's another story. (And I'm not sure we can call this a gracefully exit.)

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon May 26, 2014

Member

I don't think gracefully is good, after all it's a serious error and the app should not quit normally. The idea is also that a debugger is triggered if this happens during development.

But you're right, undefined behavior (null pointer dereferencing) is not ideal. Thinking about alternatives, std::abort() could be a better way.

Member

Bromeon commented May 26, 2014

I don't think gracefully is good, after all it's a serious error and the app should not quit normally. The idea is also that a debugger is triggered if this happens during development.

But you're right, undefined behavior (null pointer dereferencing) is not ideal. Thinking about alternatives, std::abort() could be a better way.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r May 26, 2014

Member

std::abort() sounds good to me as well, any other inputs?

Member

eXpl0it3r commented May 26, 2014

std::abort() sounds good to me as well, any other inputs?

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila May 27, 2014

Member

Why std::abort over std::exit?

Member

LaurentGomila commented May 27, 2014

Why std::abort over std::exit?

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r May 27, 2014

Member

std::exit will cause a normal application termination and since this is an actual error it shouldn't just gracefully terminate

Member

eXpl0it3r commented May 27, 2014

std::exit will cause a normal application termination and since this is an actual error it shouldn't just gracefully terminate

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs May 27, 2014

Member

abort sounds good.

Member

TankOs commented May 27, 2014

abort sounds good.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r May 28, 2014

Member

@Bromeon can you make the change to std::abort(), unless someone doesn't want it.

Member

eXpl0it3r commented May 28, 2014

@Bromeon can you make the change to std::abort(), unless someone doesn't want it.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon May 28, 2014

Member

Done.

Member

Bromeon commented May 28, 2014

Done.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r May 28, 2014

Member

Seems fine, unless someone has anything to add, I'll merge this in a bit.

Member

eXpl0it3r commented May 28, 2014

Seems fine, unless someone has anything to add, I'll merge this in a bit.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon May 28, 2014

Member

Do you want a fast-forward merge? Should I squash the commits into one?

Member

Bromeon commented May 28, 2014

Do you want a fast-forward merge? Should I squash the commits into one?

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r May 28, 2014

Member

Since the change is rather small, one commit would be nice.

Member

eXpl0it3r commented May 28, 2014

Since the change is rather small, one commit would be nice.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon May 28, 2014

Member

Okay. I also rebased the commit onto master.

Member

Bromeon commented May 28, 2014

Okay. I also rebased the commit onto master.

Output error message and abort program when XOpenDisplay() fails
Fixes issue #508.

When the X11 display could not be opened, the application crashed without notice. Now, a meaningful error message is output to sf::err() and std::abort() is called, causing immediate program termination.
@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r May 28, 2014

Member

Merged in bc1127d

Member

eXpl0it3r commented May 28, 2014

Merged in bc1127d

@eXpl0it3r eXpl0it3r closed this May 28, 2014

@eXpl0it3r eXpl0it3r added the resolved label May 28, 2014

@Bromeon Bromeon deleted the bugfix/display_segfault branch May 28, 2014

@Bromeon Bromeon removed their assignment Apr 30, 2015

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