[Freeciv-Dev] Re: core file on civserver, http://civserver.freeciv.org/games/83077

["Ross W. Wetmore" <rwetmore@xxxxxxxxxxxx>]

I maybe should have been more explicit :-)

But I see you reread your mail after the finger reflex has fingered you.

The logic should maybe have one minor tweak  over simple inversion 
of the conditional ...

-  assert(plr_tile->pending_seen != 0);
+- if (plr_tile->pending_seen != 0) {
++ if (plr_tile->pending_seen <= 0) {
+    /*
+     * WARNING: This used to be an assert. Changed for S1_14 only.
+     * 
+     * We got a core dump report here before release of 1.14.0 which 
+     * could not be traced and fixed, so we do this to be on the safe 
+     * side. This fix is not in cvs head. -- Per
+     */
+    return;
+  }
   plr_tile->pending_seen -= 1;

The logic here says that the offence of miscounting the pending_seen 
bits and attempting to stop someone from seeing even when they are 
already blind is a misdeamenor and not on a par with using weapons
of mass destruction, i.e. mandatory sentence of server decapitation. 

A LOG_WARNING or LOG_VERBOSE can be added to the clause that returns 
if you want to collect statistics on the frequency of this. The 
stacktrace code from the list a month or so back can be used to
dump more information on cases that occur to see if they are all
from one source or spread around.

But I really see this as a robustness fix for CVS rather than leaving
the issue to the rate of code bitrot and diligence of developers to
watch out for corner cases in unrelated areas of code. Why not protect
the code here in one spot, rather than forcing all callers to do it 
less they risk the ultimate penalty if some loop somewhere should 
iterate once more or less than expected sometime in the future. Or
alternatively why rely on chance when you can do a simple robustness
fix without any real impact on anything.

Cheers,
RossW
=====

At 11:39 PM 02/12/08 +0000, Per I. Mathisen wrote:
>
>On Sun, 8 Dec 2002, Ross W. Wetmore wrote:
>> It is an overzealous assert.
>>
>> Asserts are bad, especially when put in by people that really don't
>> consider all possibilities, just the one they currently want to purge.
>>
>> Change the assert to "if ... return;".
>
>See attached patch. Unless I get protests, I will commit this to S1_14
>only.
>
>  - Per
>
>Attachment Converted: "c:\program files\eudora\attach\maphand1.diff"
>