Fixup exceptions in playcontroller #191

merged 20 commits into from Jun 8, 2014

1 participant

Battle for Wesnoth member

This sequence of commits attempts to refactor the engine so that at least the play controller does not use "end_level_exception" and "end_turn_exception" for normal flow control. This helps us to simplify the normal working of the engine and make it less opaque, and might help us to safely use RAII objects for networking at least for normal purposes.

The first step is to introduce an appropriate type "possible_end_play_signal",
which covers both end_level_exception and end_turn_exception. This is intended
to become the return value of void member functions of play_controller which
previously would throw these exceptions up the call stack.

The first commit also introduces macros which make it easy to catch the exceptions,
convert them to the signal type, and return, and also to propogate the returned
signal to the caller.

The subsequent commits work from the root, at play_controller::play_scenario,
wrapping possible exception sources and handling the signal type instead. We
work incrementally, testing at each step to avoid breaking anything. Once
a function has been converted, we can work to convert the functions it calls,
and then switch calls to those functions from being wrapped in the "HANDLE"
macro to being wrapped in the "PROPOGATE" macro which is much cheaper
than an exception throw and doesn't carry the dangers of crashing the program
if a destructor somewhere throws an exception.

I suppose that if we continue this way, we could push all the throws into the
event handler and the replay module. Perhaps could continue from there,
but likely at some point the exceptions might be the simpler solution esp.
for breaking recursion in WML / lua or things like this.

cbeck88 added some commits Jun 4, 2014
@cbeck88 cbeck88 add end_play_signal type and visitors
This commit adds an "end_play_signal" type. It is meant to be used
in play_controller classes to prevent crashes due to bad exception
handling / collisions, and should help us to make assumptions more
easily about when networking operations will happen

It also adds some macros to avoid the proliferation of unreadable
try catch blocks. It is expected that many of these can be
expanded and simplified after the refactor.
@cbeck88 cbeck88 push end_play exceptions down one level in playsingle_controller
This uses the macros introduced to handle game end exceptions.
It should be a strict refactor.

Rebased to accomodate conflict here: wesnoth@516206a
Still a strict refactor.

If end_turn_exception propogates to the top of
playsingle_controller, there is no handler beyond that and the
program will terminate. If an end turn signal gets there, we
also will terminate. This commit adds an assertion failure with
a message clarifying what happened.
@cbeck88 cbeck88 push end_play_exceptions one down in call stack, at "play_turn()"
Continues trend of previous commit. play_turn becomes no throw
with respect to these exceptions, and so we can switch calls to it
to use the "PROPOGATE_END_PLAY_SIGNAL" instead of "HANDLE_...".

It looks like by progressively doing this and unit testing at each
step, we will be able to successfully convert all of the functions
in play_controller to use PROPOGATE, which is a cheap if else and
not an exception handler, and increase the stability of the engine
by avoiding the use of exceptions for control flow. If we continue
this way, we could push all the throws into the event handler
and the replay module. Perhaps could continue from there, but
likely at some point the exceptions might be the simpler solution
esp. for breaking recursion in WML / lua or things like this.
@cbeck88 cbeck88 remove unnecessary handler macro from events::raise_draw_event
After discussion on irc, it seems that this function doesn't
throw end level or end turn exceptions, and some play testing
suggests that removing the handler macros here is alright.
@cbeck88 cbeck88 push end play exceptions out of play_controller::check_time_over cb9a84b
@cbeck88 cbeck88 finish_turn() cannot throw end play exceptions, remove handlers 570e0a4
@cbeck88 cbeck88 remove handler from finish_side_done
This function won't throw end level or end turn. game_event pump
will not throw these, although the SDL event pump may throw them
in response to keyboard / menu events.
@cbeck88 cbeck88 push end play exceptions out of replay controller top level
This commit starts the process for replay_controller at the
play_replay_level function.
@cbeck88 cbeck88 push end play exceptions out of replay_controller::play_replay
Returns a "possible_end_play_signal" now instead. This also
required changes to the header in hotkey/command_executor. This
seems pretty minor, and could perhaps be refactored later.
@cbeck88 cbeck88 remove unnecessary pure virtual function definition
This is overrided differently in playsingle, playmp, and replay
controllers, and never is it called abstractly, which is what
this virtual declaration would suggest.
@cbeck88 cbeck88 push end play exceptions out of play_side()
This changes both the playsingle and playmp implementations,
but not the replay_controller implementation which is separate.

( squashed: fixup an oversight in replay_controller::play_side )
@cbeck88 cbeck88 replay_next_turn signals rather than throws, in replay_controller 34d294e
@cbeck88 cbeck88 replay_next_side signals rather than throws, in replay_controller ccbe0b5
@cbeck88 cbeck88 play_turn signals instead of throwing in replay_controller 08edc77
@cbeck88 cbeck88 replay_controller::play_side signals rather than throws 2034f06
@cbeck88 cbeck88 play_controller::init_side signals rather than throws de026e6
@cbeck88 cbeck88 play_network_turn now signals rather than throws 154f3db
@cbeck88 cbeck88 play_human_turn signals rather than throwing 13f5b27
@cbeck88 cbeck88 play_idle_loop signals rather than throwing 29ad76c
@cbeck88 cbeck88 before_human_turn signals rather than throwing 5e0f420
@cbeck88 cbeck88 merged commit 9da0b4c into wesnoth:master Jun 8, 2014

1 check passed

Details continuous-integration/travis-ci The Travis CI build passed
@cbeck88 cbeck88 deleted the cbeck88:fixup_exceptions branch Jun 8, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment