[Freeciv-Dev] Re: resubmission: bug in vnotify_conn_ex (PR#1185)

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

At 03:06 PM 02/07/29 -0700, jdorje@xxxxxxxxxxxxxxxxxxxxx wrote:
>--- "Ross W. Wetmore" <rwetmore@xxxxxxxxxxxx> wrote:
>> At 11:46 AM 02/07/20 -0700,
>> jdorje@xxxxxxxxxxxxxxxxxxxxx wrote:
>> >This is a resubmission of the patch to fix a future
>> bug in vnotify_conn_ex.
[...]
>> >- (-1,-1) is a special case meaning that the event
>> has no associated 
>> >coordinates. 
>> 
>> This should be trapped next and passed through.
>
>Correct.
>
>> > In the future it may be possible for (-1,-1) to be
>> a real 
>> >position; however, it is assumed that it will not
>> be a normal position 
>> >(otherwise this code will completely break). 
>> 
>> Incorrect, or only poorly thought through. If
>> (-1,-1) is anything but a 
>> special case, it will need to be dealt with
>> consistently throughout the
>> code. Presumably, a new flag value will be
>> implemented, but it doesn't
>> matter - that should be left to the person fixing
>> things at that time.
>
>(-1,-1) must remain a non-normal position to stay a
>special case without changing the rest of the code. 
>This is what the assertion is intended to guarantee.

Absolute balderdash ... 

Don't kill programs and break backwards compatibility just
for the excuse to push your normal jihad. Let the person
who finally does finally fix things make their own decisions
when there is a need.

In the meantime, (-1,-1) is always passed through with value
(-1,-1) whether it is "special" or "real". This is what the
code currently expects and handles. Throwing and assert will
unnecessarily take down the server.

The only fix needed beside reordering the checks, is to do a
proper normalize_map_pos() instead of an is_real() to fix the 
bug introduced when you took away the downsteam normalization.

>Your alternative does nothing to detect the bugs I
>describe above. 

Correct. It runs in a perfectly happy backwards compatible
mode without trashing the server. This is because there are
no bugs, only religion involved here.

Your code trashes the server under existing valid conditions.

This is very bad ...

> It runs happily either because there
>are no bugs of this type in the code and/or because
>the bugs are nearly undetectible without the
>assertions.

The bugs are only visible to those with religious filters in 
place, methinks, so fixing them should not be a high priority :-)

>jason
>__________________________________________________