widelands

Merge lp://staging/~widelands-dev/widelands/bug-1636966-one-soldier-crash into lp://staging/widelands/build19

  1. bug-1636966-one-soldier-crash
  2. Merge into b19
Proposed by GunChleoc
Status: Rejected
Rejected by: GunChleoc
Proposed branch: lp://staging/~widelands-dev/widelands/bug-1636966-one-soldier-crash
Merge into: lp://staging/widelands/build19
Diff against target: 100 lines (+22/-16)
1 file modified
src/logic/map_objects/tribes/soldier.cc (+22/-16)
To merge this branch: bzr merge lp://staging/~widelands-dev/widelands/bug-1636966-one-soldier-crash
Reviewer Review Type Date Requested Status
Widelands Developers Pending
Review via email: mp+309445@code.staging.launchpad.net

Commit message

Abort battle if opponent is nullptr to avoid segfaults.

Description of the change

There is no easy way to reproduce this, so I have followed the solution suggested in the bug.

To post a comment you must log in.
Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 1524. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/171031811.
Appveyor build 1367. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1636966_one_soldier_crash-1367.

Revision history for this message
SirVer (sirver) wrote :

I feel uneasy getting this into b19.

We do not know what the root cause of the bug is - why the other soldier can be zero. We know it happens - which breaks our codes assumptions already. This branch deals with this unexpected situation, potentially carrying it further and masking more bugs down the line. My argument is that crashing early is way better than hiding further bugs.

A quick qblame showed me that this code was introduced in r5877 in 2011 to fix bug 612348 - to which we also did not have a good understanding it seems. It seems this bug has been around sufficiently long to not warrant this fix for b19.

Agreed?

Revision history for this message
GunChleoc (gunchleoc) wrote :

You have convinced me.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: