[Freeciv-Dev] Re: [Patch] Stricter input checking (PR#1764)

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

Resending bounced mail ...

>The original message was received at Sat, 27 Jul 2002 09:10:54 -0700 (PDT)
>
>   ----- The following addresses had permanent fatal errors -----
><freeciv-dev@xxxxxxxxxxx>
>
>   ----- Transcript of session follows -----

>Date: Sat, 27 Jul 2002 11:16:04 -0400
>To: jdorje@xxxxxxxxxxxxxxxxxxxxx, jdorje@xxxxxxxxxxxxxxxxxxxxx
>From: "Ross W. Wetmore" <rwetmore@xxxxxxxxxxxx>
>Subject: Re: [Freeciv-Dev] Re: [Patch] Stricter input checking (PR#1764)
>Cc: freeciv-dev@xxxxxxxxxxx
>In-Reply-To: <3D3994C2.308@xxxxxxxxxxx>
>References: <3.0.6.32.20020718222151.0095e8e0@xxxxxxxxxxxxxxxxx>
>Mime-Version: 1.0
>Content-Type: text/plain; charset="us-ascii"
>
>At 12:50 PM 02/07/20 -0400, Jason Short wrote:
>>Ross W. Wetmore wrote:
>> > This is a foolish mistake.
>> >
>> > In general, requiring realness in virtually all cases is not bad.
>> > Requiring normalized in any but carefully orchestrated ones is very
>> > bad.
>> >
>> > Since normalize_map_pos() not only does a fast check for real,
>>
>>Theoretically, is_normal_map_pos is faster than normalize_map_pos.
>
>In all practical code to date, is_normal_map_pos() uses normalize_map_pos
>and diffs the result. A properly writtin normalized_map_pos() rather than
>the CVS example, does the correct tests in the correct order before
>doing any real work and is always going to be as fast or faster in the
>cases that is_normal_map_pos() deals with.
>
>Just running normalize_map_pos() and *returning* the result is always
>going to be faster, more robust and cleaner when topology stuff goes
>in and "normalized" may be a variable condition :-).
>
>> > It is an
>> > incomplete test for validity. It's use here is thus still invalid.
>> >
>> > Realness of coordinates is a hard condition that can only be dealt
>> > with at source and is an error elsewhere. Normalized is a soft and
>> > easily remedied one that can be handled anywhere in code - for example
>> > as local efficiency mods. The first *is* required of game coordinates.
>> > The second is meaningless by itself as an incomplete test.
>>
>>This is true but not particularly meaningful.  As long as "normal" has a
>>consistent meaning within the code, assuming coordinates have been
>>normalized and just doing the is_normal check can save time and
>>(occasionally) code.
>
>You are wrong on all but the first of 4 assertions as you have coded the 
>opposite. So correctly following your logic above you should take all the 
>is_normal_map_pos code out and go back to the earlier versions everywhere
>as has been suggested numerous times :-).
>
>Note even the title of this thread "stricter" is an oxymoron. You are
>removing the stricter correct checks in favour of more expensive but
>incomplete and hence generally useless ones. 
>
>> > Normalized
>> > is also not a rigorously defined concept while real is. Thus "passing
>> > normalized coordinates" is only useful wrt a local definition of
>> > normalized.
>>
>>This is a very valid point, and a decent reason not to use 
>>is_normal_map_pos in this case.  Since this code interfaces the client 
>>with the server, the use of is_normal here assumes that the client and 
>>server have the same definitions of "normal" - which is probably a safe 
>>assumption, but an unnecessary one.
>
>Building assumptions is a mistake when you can do it faster and more
>rigorously without the risk. This is why I say you are setting yourself 
>up for future void_tile issues. The number of cores and segfaults in the
>current release and CVS code over the last year should be an indication
>of how well some people are able to judge these things.
>
>Note, doing a normalze_map_pos() at source will handle the realness check
>and provide you with normal coordinates within the current topology
>context. Listing *certain* functions as requiring normalized coordinates
>as defined in the current context as an explicit part of the interface is 
>fine. 
>
>But sometimes you don't want or need this. Insisting that all coordinates 
>need to be normalized then removing all the normalized_map_pos() checks 
>everywhere and replacing them with a more expensive and faulty is_normal
>test, including portal points like input from the client where not only 
>disimilar clients but network errors or other conditions can alter data 
>is *still* a foolish mistake.
>
>>jason
>
>Cheers,
>RossW
>=====