bugFreeciv - Bugs: bug #20744, Recursive wipe_unit() saving...

 
 
Show feedback again

bug #20744: Recursive wipe_unit() saving drowning units from calling wipe_unit()

Submitted by:  Marko Lindqvist <cazfi>
Submitted on:  Fri 19 Apr 2013 08:52:39 AM UTC  
 
Category: generalSeverity: 3 - Normal
Priority: 5 - NormalStatus: Fixed
Assigned to: Marko Lindqvist <cazfi>Open/Closed: Closed
Release: Operating System: None
Planned Release: 2.4.0-beta2, 2.5.0, 2.6.0

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Tue 23 Apr 2013 05:10:17 PM UTC, SVN revision 22752:

Store units unloaded from dying transport and potentially drowning
to unit list instead of just having counter of such units in the tile.
This avoids problems with recursive calls from handling units that
calling wipe_unit() expects to handle.

Patch by Emmet Hikory with minor updates by me

See bug #20744

(Browse SVN revision 22752)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Tue 23 Apr 2013 05:10:10 PM UTC, SVN revision 22751:

Store units unloaded from dying transport and potentially drowning
to unit list instead of just having counter of such units in the tile.
This avoids problems with recursive calls from handling units that
calling wipe_unit() expects to handle.

Patch by Emmet Hikory with minor updates by me

See bug #20744

(Browse SVN revision 22751)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sun 21 Apr 2013 09:12:05 PM UTC, comment #5:

I think it's safe either way: I just tend to avoid having unused arguments or functions if possible, to ease later reading of the code (but given the volume of testing to which this was subjected, there is a strong argument for leaving it intact).

Looking at the most recent revisions of the patch, I wonder if unit_list_iterate_safe(remaining, pcargo){} should be guarded with a check to ensure unit_list_size(remaining) > 0 (this would apply to either S2_4 or trunk patches).

Emmet Hikory <persia>
Project Member
Sun 21 Apr 2013 07:44:41 PM UTC, comment #4:

Correction: Had to make new version anyway due to other changes to codebase, so removed S2_4 helpless while at it.

(file #17791, file #17792)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sun 21 Apr 2013 07:35:49 PM UTC, comment #3:

As we're heading to beta2, I'll commit this with extra argument intact. That's what's been extensively tested (like in: several autogames (on multicore system) constantly running almost from the day trunk patch was first introduced). We may cleanup it away from S2_4 version after the beta, if someone bothers.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Fri 19 Apr 2013 09:28:39 AM UTC, comment #2:

try_to_save_unit() doesn't need to take the helpless argument for S2_4, as this only exists as a base for future extension, but leaving it there should cause no problem and it may be helpful to have the S2_4 and S2_5 functions be identical to reduce rework if an issue is discovered.

Emmet Hikory <persia>
Project Member
Fri 19 Apr 2013 09:12:38 AM UTC, comment #1:

- S2_4 version of the patch

(file #17768)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Fri 19 Apr 2013 08:52:39 AM UTC, original submission:

Haven't tried to setup test case, but when reading wipe_unit() refactoring from patch #3804 (use unit lists to store drowning units instead of just having count of them and then rechecking which units they are) it occurred to me that recursively called wipe_unit() might handle those units calling wipe_unit() has counted to its 'drowning' counter, meaning that the counter value is not correct (in practice: counter instructs to wipe units as drowning ones even after all have been saved)

Using the lists the way refactoring part of patch #3804 does avoids such problems.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.

 

(Note: upload size limit is set to 1024 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #17791:  WipeUnitLists-2.patch added by cazfi (8kB - text/x-diff)
file #17768:  WipeUnitLists-S2_4.patch added by cazfi (8kB - text/x-diff)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by persia (Posted a comment)
  • -unavailable- added by cazfi (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 7 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Tue 23 Apr 2013 05:10:28 PM UTCcazfiStatusReady For Test=>Fixed
      Assigned toNone=>cazfi
      Open/ClosedOpen=>Closed
    Sun 21 Apr 2013 07:44:41 PM UTCcazfiAttached File-=>Added WipeUnitLists-2.patch, #17791
      Attached File-=>Added WipeUnitLists-S2_4-2.patch, #17792
      Planned Release2.4.0, 2.5.0, 2.6.0=>2.4.0-beta2, 2.5.0, 2.6.0
    Fri 19 Apr 2013 09:12:37 AM UTCcazfiAttached File-=>Added WipeUnitLists-S2_4.patch, #17768
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup