Skip to content
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.

Sign up for GitHub

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

Jump to bottom

remove throw; workaround for rv_api_1 #535

Closed
wants to merge 1 commit into from
Closed

remove throw; workaround for rv_api_1 #535

wants to merge 1 commit into from

Conversation

caiiiycuk
Copy link
Contributor

  1. throw is removed because try/catch is not used, and it helps to keep web binary as small as possible
  2. rv_api_1 can't be linked using this style for now, so disabled for emscripten

@lpenguin
Copy link
Contributor

lpenguin commented Jan 27, 2022 •
edited
Loading

rv_api_1 can't be linked using this style for now, so disabled for emscripten

Can we put the rv_api check under ifdef EMSCRIPTEN? It would be nice to have the API version check in cases where we can afford it

Update: Ah, exactly, you already did it :)

@caiiiycuk
Copy link
Contributor Author

Already ifdefed

lpenguin
lpenguin reviewed Jan 27, 2022
View reviewed changes
lib/xgraph/xgraph.cpp
@@ -132,7 +132,7 @@ int XGR_Screen::init(int x,int y,int flags_in)
std::cout<<"XGR_Screen::init"<<std::endl;
// Init SDL video
if (XGR_ScreenSurface==NULL) {
if (SDL_Init(SDL_INIT_EVERYTHING) < 0) {
if (SDL_Init(SDL_INIT_VIDEO | SDL_INIT_TIMER) < 1) {
Copy link
Contributor

@lpenguin lpenguin Jan 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be SDL_Init(...) < 0? It should return 0 on success and negative value on failure. See https://wiki.libsdl.org/SDL_Init

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep

@caiiiycuk caiiiycuk marked this pull request as draft January 29, 2022 06:32
@caiiiycuk
Copy link
Contributor Author

replaced with #540

@caiiiycuk caiiiycuk closed this Jan 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpenguin lpenguin lpenguin left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@caiiiycuk @lpenguin