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

Remove boost dependency #842

Merged
merged 16 commits into from Apr 1, 2016

Conversation

Projects
None yet
5 participants
@Zegeri
Member

Zegeri commented Mar 22, 2016

It will probably conflict with other branches. This doesn't have that high priority, so please attend first other branches and I'll rebase this one.

cache(System2)
cache(Frame)
cache(Title)
cache(System)

This comment has been minimized.

@Ghabry

Ghabry Mar 22, 2016

Member

Imo this could be even replaced with real function definitions. Is redundant but more clear then this (or that ugly boost pp). What do you think?

@Ghabry

Ghabry Mar 22, 2016

Member

Imo this could be even replaced with real function definitions. Is redundant but more clear then this (or that ugly boost pp). What do you think?

@@ -110,7 +110,7 @@ void Game_System::SePlay(RPG::Sound const& se) {
return;
FileRequestAsync* request = AsyncHandler::RequestFile("Sound", se.name);
se_request_ids[se.name] = request->Bind(boost::bind(&Game_System::OnSeReady, _1, se.volume, se.tempo));
se_request_ids[se.name] = request->Bind(std::bind(&Game_System::OnSeReady, std::placeholders::_1, se.volume, se.tempo));

This comment has been minimized.

@Ghabry

Ghabry Mar 22, 2016

Member

Does this still need std::bind? At other location you use normal function calls now.

@Ghabry

Ghabry Mar 22, 2016

Member

Does this still need std::bind? At other location you use normal function calls now.

This comment has been minimized.

@Zegeri

Zegeri Mar 22, 2016

Member

Yes, because OnSeReady isn't a member function of a class like the rest of them, so it can't use the FileRequestAsync::Bind for member classes with variadic parameters. To do it without std::bind, I'd have to templatize the other FileRequestAsync::Bind for just this case.

@Zegeri

Zegeri Mar 22, 2016

Member

Yes, because OnSeReady isn't a member function of a class like the rest of them, so it can't use the FileRequestAsync::Bind for member classes with variadic parameters. To do it without std::bind, I'd have to templatize the other FileRequestAsync::Bind for just this case.

Zegeri added some commits Mar 12, 2016

Show outdated Hide outdated src/audio_al.cpp
FLUID_FAILED);
if (fluid_synth_sfload(synth.get(), getenv("DEFAULT_SOUNDFONT"), 1) == FLUID_FAILED) {
Output::Error("Couldn't load soundfont\n%s.", getenv("DEFAULT_SOUNDFONT"));
return;

This comment has been minimized.

@scurest

scurest Mar 25, 2016

Contributor

This return is unreachable. While you're at it, can you mark Output::Error and Output::ErrorStr with [[noreturn]]?

@scurest

scurest Mar 25, 2016

Contributor

This return is unreachable. While you're at it, can you mark Output::Error and Output::ErrorStr with [[noreturn]]?

This comment has been minimized.

@fdelapena

fdelapena Mar 25, 2016

Contributor

In this particular case it could be a warning instead to make the return reachable.

@fdelapena

fdelapena Mar 25, 2016

Contributor

In this particular case it could be a warning instead to make the return reachable.

This comment has been minimized.

@Zegeri

Zegeri Mar 27, 2016

Member

@fdelapena It isn't prepared to deal with a non-working fluidsynth, so it might be better to throw an error until audio_al gets a big overhaul.

@Zegeri

Zegeri Mar 27, 2016

Member

@fdelapena It isn't prepared to deal with a non-working fluidsynth, so it might be better to throw an error until audio_al gets a big overhaul.

@Zegeri Zegeri changed the title from Drop boost dependency to Reduce boost dependency Mar 26, 2016

@Zegeri

This comment has been minimized.

Show comment
Hide comment
@Zegeri

Zegeri Mar 26, 2016

Member

Here are the commits I've picked out.
d8b7a4f is an attempt to implement those needed codecvt facets for the C++ libraries without the codecvt header. It's based on the libc++ implementation. Sadlly, it isn't enough to make it work. If we went through that route we would also need to add an implementation of wstring_convert.

Member

Zegeri commented Mar 26, 2016

Here are the commits I've picked out.
d8b7a4f is an attempt to implement those needed codecvt facets for the C++ libraries without the codecvt header. It's based on the libc++ implementation. Sadlly, it isn't enough to make it work. If we went through that route we would also need to add an implementation of wstring_convert.

@Zegeri

This comment has been minimized.

Show comment
Hide comment
@Zegeri

Zegeri Mar 27, 2016

Member

Please, do not merge yet. I got an invalid UTF-8 sequence error when testing #849.

terminate called after throwing an instance of 'boost::exception_detail::clone_impl<boost::exception_detail::error_info_injector<std::out_of_range> >'
  what():  Invalid UTF-8 sequence encountered while trying to encode UTF-32 character

The codecvt branch returns:

terminate called after throwing an instance of 'std::range_error'
  what():  wstring_convert::from_bytes
Member

Zegeri commented Mar 27, 2016

Please, do not merge yet. I got an invalid UTF-8 sequence error when testing #849.

terminate called after throwing an instance of 'boost::exception_detail::clone_impl<boost::exception_detail::error_info_injector<std::out_of_range> >'
  what():  Invalid UTF-8 sequence encountered while trying to encode UTF-32 character

The codecvt branch returns:

terminate called after throwing an instance of 'std::range_error'
  what():  wstring_convert::from_bytes
Show outdated Hide outdated pr
@@ -0,0 +1 @@
Subproject commit cddde33722ba698571f93f5493bb114bdba69a16

This comment has been minimized.

@Ghabry

Ghabry Mar 27, 2016

Member

Any idea what that is?

@Ghabry

Ghabry Mar 27, 2016

Member

Any idea what that is?

This comment has been minimized.

@Zegeri

Zegeri Mar 27, 2016

Member

Ouch, a git worktree. I'll do a rebase. Sorry for abusing Jenkins.

@Zegeri

Zegeri Mar 27, 2016

Member

Ouch, a git worktree. I'll do a rebase. Sorry for abusing Jenkins.

@Ghabry

This comment has been minimized.

Show comment
Hide comment
@Ghabry

Ghabry Mar 28, 2016

Member

Is that one ready or do you still have unicode errors?

Works for me under Windows. Also Name input :)

Member

Ghabry commented Mar 28, 2016

Is that one ready or do you still have unicode errors?

Works for me under Windows. Also Name input :)

@Ghabry

This comment has been minimized.

Show comment
Hide comment
@Ghabry

Ghabry Mar 28, 2016

Member

Maybe @fdelapena has an idea why the emscripten build is broken
"�[0;33mwarning:�[0m incorrect target triple '' (did you use emcc/em++ on all source files and not clang directly?)" ?

Member

Ghabry commented Mar 28, 2016

Maybe @fdelapena has an idea why the emscripten build is broken
"�[0;33mwarning:�[0m incorrect target triple '' (did you use emcc/em++ on all source files and not clang directly?)" ?

@fdelapena

This comment has been minimized.

Show comment
Hide comment
@fdelapena

fdelapena Mar 29, 2016

Contributor

Maybe @fdelapena has an idea why the emscripten build is broken
"�[0;33mwarning:�[0m incorrect target triple '' (did you use emcc/em++ on all source files and not clang directly?)" ?

It throws exception because index-pr.html.mem is not in the folder (404). This file is being generated when using emcc with -O2, but liblcf-js-pr and player-js-pr were built with -O0 (it even might be unrelated, though). When trying to do a clean rebuild of liblcf-js-pr and player-js-pr with -O2, now player-js-pr fails with liblcf 0.4.1 fixups (?).

Contributor

fdelapena commented Mar 29, 2016

Maybe @fdelapena has an idea why the emscripten build is broken
"�[0;33mwarning:�[0m incorrect target triple '' (did you use emcc/em++ on all source files and not clang directly?)" ?

It throws exception because index-pr.html.mem is not in the folder (404). This file is being generated when using emcc with -O2, but liblcf-js-pr and player-js-pr were built with -O0 (it even might be unrelated, though). When trying to do a clean rebuild of liblcf-js-pr and player-js-pr with -O2, now player-js-pr fails with liblcf 0.4.1 fixups (?).

@Ghabry

This comment has been minimized.

Show comment
Hide comment
@Ghabry

Ghabry Mar 29, 2016

Member

New emscripten status: assert on startup

Member

Ghabry commented Mar 29, 2016

New emscripten status: assert on startup

@Ghabry

This comment has been minimized.

Show comment
Hide comment
@Ghabry

Ghabry Mar 29, 2016

Member

No idea what's going on with picojson that breaks emscripten but this is not zegeris fault.
So I vote LGTM 👍 if it is ready?

Member

Ghabry commented Mar 29, 2016

No idea what's going on with picojson that breaks emscripten but this is not zegeris fault.
So I vote LGTM 👍 if it is ready?

@Ghabry Ghabry changed the title from Reduce boost dependency to Remove boost dependency Mar 30, 2016

@carstene1ns

This comment has been minimized.

Show comment
Hide comment
@carstene1ns

carstene1ns Mar 31, 2016

Member

btw. we are currently hitting a similar error in an emscripten build without your pull request applied, so it looks unrelated.
Game is "The Gray Garden", the build is from current master commit (ba1f269).

uncaught exception: abort() at jsStackTrace@https://easy-rpg.org/play/index-pr.js:1248:13
stackTrace@https://easy-rpg.org/play/index-pr.js:1265:22
abort@https://easy-rpg.org/play/index-pr.js:875474:44
_abort@https://easy-rpg.org/play/index-pr.js:8391:7
_free@https://easy-rpg.org/play/index-pr.js:400786:27
__ZNSt3__112basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEE9__grow_byEjjjjjj [std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::__grow_by(unsigned int, unsigned int, unsigned int, unsigned int, unsigned int, unsigned int)]@https://easy-rpg.org/play/index-pr.js:802845:30
__ZN8picojson13_parse_stringINSt3__112basic_stringIcNS1_11char_traitsIcEENS1_9allocatorIcEEEENS1_19istreambuf_iteratorIcS4_EEEEbRT_RNS_5inputIT0_EE [bool picojson::_parse_string<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::istreambuf_iterator<char, std::__1::char_traits<char> > >(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >&, picojson::input<std::__1::istreambuf_iterator<char, std::__1::char_traits<char> > >&)]@https://easy-rpg.org/play/index-pr.js:295865:7
__ZN8picojson6_parseINS_21default_parse_contextENSt3__119istreambuf_iteratorIcNS2_11char_traitsIcEEEEEEbRT_RNS_5inputIT0_EE [bool picojson::_parse<picojson::default_parse_context, std::__1::istreambuf_iterator<char, std::__1::char_traits<char> > >(picojson::default_parse_context&, picojson::input<std::__1::istreambuf_iterator<char, std::__1::char_traits<char> > >&)]@https://easy-rpg.org/play/index-pr.js:246537:10
__ZN8picojson6_parseINS_21default_parse_contextENSt3__119istreambuf_iteratorIcNS2_11char_traitsIcEEEEEEbRT_RNS_5inputIT0_EE [bool picojson::_parse<picojson::default_parse_context, std::__1::istreambuf_iterator<char, std::__1::char_traits<char> > >(picojson::default_parse_context&, picojson::input<std::__1::istreambuf_iterator<char, std::__1::char_traits<char> > >&)]@https://easy-rpg.org/play/index-pr.js:246964:12
__ZN10Scene_Logo12OnIndexReadyEP17FileRequestResult [Scene_Logo::OnIndexReady(FileRequestResult*)]@https://easy-rpg.org/play/index-pr.js:302316:11
__ZN5boost6detail8function26void_function_obj_invoker1INSt3__19binder1stINS3_10mem_fun1_tIv10Scene_LogoP17FileRequestResultEEEEvS8_E6invokeERNS1_15function_bufferES8_ [boost::detail::function::void_function_obj_invoker1<std::__1::binder1st<std::__1::mem_fun1_t<void, Scene_Logo, FileRequestResult*> >, void, FileRequestResult*>::invoke(boost::detail::function::function_buffer&, FileRequestResult*)]@https://easy-rpg.org/play/index-pr.js:823357:3
__ZN16FileRequestAsync13CallListenersEb [FileRequestAsync::CallListeners(bool)]@https://easy-rpg.org/play/index-pr.js:457901:5
__ZN16FileRequestAsync12DownloadDoneEb [FileRequestAsync::DownloadDone(bool)]@https://easy-rpg.org/play/index-pr.js:772514:2
__ZN12_GLOBAL__N_116download_successEjPvPKc [(anonymous namespace)::download_success(unsigned int, void*, char const*)]@https://easy-rpg.org/play/index-pr.js:848342:2
dynCall_viii@https://easy-rpg.org/play/index-pr.js:850505:2
Runtime.dynCall@https://easy-rpg.org/play/index-pr.js:515:14
http_onload@https://easy-rpg.org/play/index-pr.js:10178:1
Member

carstene1ns commented Mar 31, 2016

btw. we are currently hitting a similar error in an emscripten build without your pull request applied, so it looks unrelated.
Game is "The Gray Garden", the build is from current master commit (ba1f269).

uncaught exception: abort() at jsStackTrace@https://easy-rpg.org/play/index-pr.js:1248:13
stackTrace@https://easy-rpg.org/play/index-pr.js:1265:22
abort@https://easy-rpg.org/play/index-pr.js:875474:44
_abort@https://easy-rpg.org/play/index-pr.js:8391:7
_free@https://easy-rpg.org/play/index-pr.js:400786:27
__ZNSt3__112basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEE9__grow_byEjjjjjj [std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::__grow_by(unsigned int, unsigned int, unsigned int, unsigned int, unsigned int, unsigned int)]@https://easy-rpg.org/play/index-pr.js:802845:30
__ZN8picojson13_parse_stringINSt3__112basic_stringIcNS1_11char_traitsIcEENS1_9allocatorIcEEEENS1_19istreambuf_iteratorIcS4_EEEEbRT_RNS_5inputIT0_EE [bool picojson::_parse_string<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::istreambuf_iterator<char, std::__1::char_traits<char> > >(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >&, picojson::input<std::__1::istreambuf_iterator<char, std::__1::char_traits<char> > >&)]@https://easy-rpg.org/play/index-pr.js:295865:7
__ZN8picojson6_parseINS_21default_parse_contextENSt3__119istreambuf_iteratorIcNS2_11char_traitsIcEEEEEEbRT_RNS_5inputIT0_EE [bool picojson::_parse<picojson::default_parse_context, std::__1::istreambuf_iterator<char, std::__1::char_traits<char> > >(picojson::default_parse_context&, picojson::input<std::__1::istreambuf_iterator<char, std::__1::char_traits<char> > >&)]@https://easy-rpg.org/play/index-pr.js:246537:10
__ZN8picojson6_parseINS_21default_parse_contextENSt3__119istreambuf_iteratorIcNS2_11char_traitsIcEEEEEEbRT_RNS_5inputIT0_EE [bool picojson::_parse<picojson::default_parse_context, std::__1::istreambuf_iterator<char, std::__1::char_traits<char> > >(picojson::default_parse_context&, picojson::input<std::__1::istreambuf_iterator<char, std::__1::char_traits<char> > >&)]@https://easy-rpg.org/play/index-pr.js:246964:12
__ZN10Scene_Logo12OnIndexReadyEP17FileRequestResult [Scene_Logo::OnIndexReady(FileRequestResult*)]@https://easy-rpg.org/play/index-pr.js:302316:11
__ZN5boost6detail8function26void_function_obj_invoker1INSt3__19binder1stINS3_10mem_fun1_tIv10Scene_LogoP17FileRequestResultEEEEvS8_E6invokeERNS1_15function_bufferES8_ [boost::detail::function::void_function_obj_invoker1<std::__1::binder1st<std::__1::mem_fun1_t<void, Scene_Logo, FileRequestResult*> >, void, FileRequestResult*>::invoke(boost::detail::function::function_buffer&, FileRequestResult*)]@https://easy-rpg.org/play/index-pr.js:823357:3
__ZN16FileRequestAsync13CallListenersEb [FileRequestAsync::CallListeners(bool)]@https://easy-rpg.org/play/index-pr.js:457901:5
__ZN16FileRequestAsync12DownloadDoneEb [FileRequestAsync::DownloadDone(bool)]@https://easy-rpg.org/play/index-pr.js:772514:2
__ZN12_GLOBAL__N_116download_successEjPvPKc [(anonymous namespace)::download_success(unsigned int, void*, char const*)]@https://easy-rpg.org/play/index-pr.js:848342:2
dynCall_viii@https://easy-rpg.org/play/index-pr.js:850505:2
Runtime.dynCall@https://easy-rpg.org/play/index-pr.js:515:14
http_onload@https://easy-rpg.org/play/index-pr.js:10178:1
@Ghabry

This comment has been minimized.

Show comment
Hide comment
@Ghabry

Ghabry Apr 1, 2016

Member

Great find @carstene1ns
Because this is obviously a general emscripten problem and not caused by the PR at all I suggest that @Zegeri adds his rtptable commit again and then LGTM 👍.

Member

Ghabry commented Apr 1, 2016

Great find @carstene1ns
Because this is obviously a general emscripten problem and not caused by the PR at all I suggest that @Zegeri adds his rtptable commit again and then LGTM 👍.

@Ghabry Ghabry merged commit 4bd31d3 into EasyRPG:master Apr 1, 2016

5 checks passed

Android Build finished.
Details
Linux Build finished.
Details
OSX Build finished.
Details
Windows Build finished.
Details
web Build finished.
Details

@Zegeri Zegeri deleted the Zegeri:boost branch Apr 6, 2016

@Ghabry Ghabry modified the milestone: 0.4.2 Apr 11, 2016

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