[Freeciv-Dev] Re: [PATCH] Formatting cleanup.

[Gaute B Strokkenes <gs234@xxxxxxxxx>]

On Thu, 13 Sep 2001, hawk@xxxxxxxxxxxxxxxxxxxxxxx wrote:
> On Wed, Sep 12, 2001 at 11:07:46PM -0400, Ross W. Wetmore wrote:
>> Gaute
>> 
>> There was unanimous agreement from the list that your concept of
>> real map positions was badly flawed.

Firstly, unless I missed something we never had an argument about
"real".  We did have an argument about the meaning of the word
"normal", but even so Raimar and most other people agreed that the
underlying concept was a useful distinction, even if there was no
unamious vote on the name.

Off course, this is all off-topic since my patch doesn't touch on
anything called "real".

>> I fail to understand why you continue to try and apply
>> transformations to convert unreal positions to point at valid
>> border tiles or any other arbitrary tile except maybe void_tile.

I assume you're referring to nearest_real_pos() here.  I replaced
calls to map_adjust_x() and map_adjust_y() with nearest_real_pos() in
places where I judged that the use of map_adjust_y() was used
deliberatly because it simplified the algorithm or whatever, rather
than as a broken band-aid to avoid segfaults.  The process is very
much analogous to the recent drive to replace map_adjust_[xy] and
manual checks with calls to normalize_map_pos().

The next time you fail to understand why I do something, I would
suggest that you seek enlightenment in a less confrontational and
assertive manner before you demand my head on a plate.

>> In addition this patch doesn't even do a comprehensive job of
>> breaking all the locations where it is applicable.

I'm not sure what you're talking about.  Feel free to point out
anything that I've missed.

>> You have broken the implementation of normalize_map_pos by checking
>> for realness after having modified the coordinates.

Actually, the inclusion of this change was a blooper on my part; I
wasn't intending for it to go in yet, although the change is sound.
On the other hand, 

Still, the change in normalize_map_pos() was motivated by the fact
that this makes it easier to change places where we use
map_adjust_[xy] unconditionally to use normalize_map_pos() instead,
with minimal behaviour change or chance of anything breaking.  I would
not complain or worry if anyone changed it so that it sets *x and *y
to some totally bogus value when unreal coordinates are passed and
debugging is not disabled, though I don't think it's likely to make a
difference.

This change was discussed along with the rest of the previous patch,
which this one is (mostly) a subset of.  No one objected, and I find
it curious that you've waited until now to complain.

The only possible arguments against this which I can think of are a)
performance loss, which I'm pretty sure is minimal in this case, and
b) the "you cannot necessarily attach a meaning to `normal', or
`normalized', or `wrapped', or whatever you prefer to call it, when
dealing with unreal coordinates" argument.  This is also incorrect, as
I have explained on a couple of occassions with no dissenters.
Someone tried to come up with examples of strange topologies where
this is not true, but I showed that these examples were flawed in that
they broke other assumptions about the way Freeciv deals with maps, in
ways such that normalize_map_pos() does not make sense anyway.

>> And 90% of this is cosmetic reformats with zero code impact 

Congratulations, you just found out why this was labelled "formatting
cleanup".  But what qualifies as formatting cleanup?  Only that which
changes only spaces and tabs?  Is it allowed to change variable names,
or to improve comments?

Hotly insisting that the splitting out of a five-line idiom to a
separate function is so special when compared to other housekeepking
tasks that it _must_ be segregated into its own patch only makes sense
if you subscribe to an extremely formalised and rigid view of how
program development should be conducted.  It's not a view that I care
to subscribe to.

>> and negative overall value

That is your subjective opinion, which incidentally runs counter to
that of the Freeciv style guide.

> Full ack.

I don't understand you--by the time we were done discussing the
previous patch I was all set to apply it, and my only reasons for not
doing so were the fact that I had not satisfied myself that it 100%
perfect, because of some strange change in the AI (I am 99% sure this
is because the AI previously did something stupid like reading
void_tile, but I wanted to track it down nonetheless).

Your only remaining complaint was that I should separate the
formatting changes; this I have done.

>> This is not the sort of patch I would ever expect to see from a
>> Freeciv maintainer. And after submitting such a patch I would not
>> in the least be surprised if that privilege were removed.
> 
> I wouldn't go as far as this. However I think you have made a bigger
> mistake.

Personally I think Ross is just sore because I did not appreciate
everything in his "corecleanup" patch series and therefore did not
apply them.  Which reminds me: Ross, are you going to make another
version incorporating the changes that Raimar and I outlined, or
should I start thinking about doing something similar myself?

-- 
Big Gaute                               http://www.srcf.ucam.org/~gs234/
After this, I'm going to BURN some RUBBER!!