[Freeciv-Dev] Re: [PATCH] Map cleanups (PR#1208)

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Tue, Jan 08, 2002 at 11:13:28PM -0500, Ross W. Wetmore wrote:
> 
> 
> At 10:19 AM 02/01/08 +0100, Raimar Falke wrote:
> >On Mon, Jan 07, 2002 at 09:32:44PM -0500, Ross W. Wetmore wrote:
> >> 
> >> I lost you here. This is the only part that looks like a "request", 
> >> but it is confusing.
> 
> >Here we go:
> 
> Yes, it is time to get the religious debates and other blockages out in
> the open and then behind us, so we can get on with the process of fixing 
> Freeciv :-).
> 
> >> 0)  mapclean_00.dif
> >>     This is a pure cut and paste line rearrangement of the existing
> >> files intended to improve the grouping of related functionality and
> >> simplify/localize the subsequent changes. The line and character
> >> count (wc) should be unchanged. One can repeat this by doing cut and 
> >> paste operations on a clean CVS file to match side-by-side views with 
> >> the patch result, and use diff as a final verify step if you are
> >> consciencious or lacking in trust :-).
> >
> >For inclusion you have to provide at least a description of the
> >sections. For macro, enum movement a style guide fixing is necessary.
> 

> I can just embed the rearrangements in with the fixes

This may be more successful. However don't you agree that a certain
descriptions of the sections may be nice/needed to aim the unknowing
future programmer?

> as you do in your patches. This should avoid your gasping for
> context here.
> 
> Is this suitable?
> 
> >> 1)  mapclean_01.dif
> >>     This is a series of local cosmetic and minor updates which should
> >> have no global scope or significant impact on game execution. They can
> >> largely be scanned in a superficial way, i.e. no heavy thinking.
> >> 
> >
> >> a)  miscellaneous K&R2 style changes, mostly added spaces. This is not
> >>     a complete pass, but resolves discrepancies between changes done 
> >>     during the corecleanups and CVS which improve general readability.
> >
> >The "feed the whole code through indent" problem again. Can you
> >postpone these?
> 
> This reduces the differences between the corecleanups and CVS in ways
> that are consistent with K&R and the style discussions. It is done
> largely because they are differences that have been creeping in. They
> are just obvious here because I split this out from the more serious
> changes as with the above rearrangements.
> 
> This is typically done by most of your patches and those of others as 
> well, even in places where it makes the code less readable. There is 
> nothing that contravenes any guidelines or goals, so they can only 
> improve the codebase.
> 
> I assume this is suggestion, and since there is no reason to treat this
> patch any different from others, I will ignore it for the most part. 

Yes I indent the lines which I touch. However I don't indent line
which I don't touch. So I have no problems with a patch such as:

- if(40<b)
+ if(SOME_NICE_CONSTANT < b) {
     foo
+ }

> >> b)  complete map_get_tile() -> MAP_TILE() changes for consistency and
> >>     improved performance (direct array vs function call access)
> >
> >If we go for consistency I would make the use of the public function
> >map_get_tile mandatory. If we go for speed make map_get_tile a
> >macro. Problem: either you have to review all calls and uppercase it
> >(e.g. MAP_GET_TILE).
> 
> It works fine as a macro by #defining map_get_tile() in the header as
> with the other macroized forms. It is safe and has been run that way
> in the corecleanups extensively. I believe Jason also has used it.
> The change here is to remove the previous partial MAP_TILE() changes,
> i.e. convert everything back to map_get_tile().

> Uppercasing is a poor thing to do elsewhere. It involves lots of code 
> change that is unneeded and has attendant risk and disruption.

Ack.

> Here is was because there was a specific macro partially
> implemented, and the function definition needed to be left exposed.
> 
> It is always possible to run macro or function version if left in a 
> single format.
> 
> So, your suggestions here are not particularly good from a technical
> standpoint. 

> It should be left as in the patch if you just want the macro form
> here.

To be honest I'm not sure what I want. I see the performance gains by
inlining (via macro or inline function) the code. However if we go for
the macro in the _long_ run the uppercasing is necessary IMHO. The
decision (current state, access macros all over the place, introduce
inlining) is a major one and I have currently no big favorite. The
inlining however has the best benefit/effort ratio IMHO if we leave
the old compilers out. Before this decision isn't determined I will
not frivolously accept patches which touch this area.

> It should be changed to map_get_tile() in the patch otherwise, and
> the header updated if you want to use the macro everywhere. The
> latter is trivial to remove at the next flipflop.
> 
> >> c)  is_normal_map_pos() -> normalize_map_pos()
> >>     Returned values must be normalized, so just do it and return.
> >
> >No.
> 
> Neither a particularly explanatory or useful comment. There is 
> considerable debate about it, you are welcome to join in and share your
> reasons.

I have done in the previous email: spread of knowledge.

> >> d)  is_real_tile() is a test, not a full normalization, make it so.
> >
> >No.
> 
> Again, both a useless comment, and a poor technical decision. 

I have done in the previous email: spread of knowledge.

> There are test conditions for realness and a normalization process. 
> A proper interface should expose both of these, and not combine them 
> into a monolithic performance intensive whole. 
> 
> This in fact causes loops in code that correctly tests for realness 
> inside normalize_map_pos() before proceeding with the operation. Thus
> it is required for the rest of the patch.
> 
> Effectively, this is what the macros are doing, and consistency between
> the two is worthwhile for a number of reasons.

If you spread the knowledge that speed must be the reason. I think
that both the current normalize_map_pos and the new smart
is_normal_map_pos/is_real_tile have to perform the same number of test
in the average case. So the only difference is the overhead of one
function call.

> >> e)  normalize_map_pos() should test for realness before it undertakes
> >>     any transformation. This fixes any potential bugs from side effects.
> >
> >Idea yes. Implementation no.
> 
> Again a poor technical analysis. Then a contradictory response.
> 

> I will repeat, any normalize_map_pos() that performs any operation on
> unreal coordinates is repeating the void_tile mistake of changing things
> that cannot be legitimately changed.
> 
> You need to insure the coordinates are within real bounds, before you 
> can proceed with the transformation.

Ack. So "idea yes".

However "Implementation no" since the is_real_tile you used for this
would result in a recursive loop if the changes to is_real_tile
weren't applied.

> >> g)  RANGE_CHECK macros are added and used to improve performance by
> >>     reducing execution in most cases where applicable by a factor of two.
> >
> >I need numbers. It looks like I have to gather them myself. Either
> >from the mail archive or by testing.
> 
> Flipflop decisions are really a poor way to deal with such issues. Nor
> is continually revisiting old debates.
> 
> It is a bad (possibly terminal) habit for anyone in a position of authority.

> >> h)  Wrap and clipping macros are added to standardize code use. Overtime
> >>     all map code that does local flavours should be updated to use these.
> >
> >No.
> 
> see above
> 
> >> i)  map_adjust_x() and map_adjust_y() are redefined to be explicit
> >>     wrap and clip operations for the standard cylindrical map, as the
> >>     first instance of standardization :-).
> >
> >No.
> 
> see above

Also from an old email: the number of new code lines and macros are
much to big relating to the importance of map_adjust_[xy]. Note that
map_adjust_[xy] should die.

> >> j)  map_to_index() is defined as the start of the map_inx() removal.
> >>     When the corecleanup and topology fixes are all applied, this will
> >>     be completely gone. index_to_map_pos() as the reverse operation is
> >>     defined in the standard manner, i.e. returns updated coordinates
> >>     and a status return indicating success or failure.
> >
> >Yes. But rename it in all the code. Where would index_to_map_pos be
> >needed?
> 
> This is the patch for map.c and map.h which are "file" fixes as you 
> requested. The later patches will deal with other followon changes.
> There are only 3 or 4 routines that use map_inx() anyway.
> 
> These are all done in the alternate devpath, though I initially used
> map_index() as suggested by previous discussion, and have changed this 
> to the more or less standard format for coordinates for consistency in 
> the final CVS version.
> 
> whole_map_iterate, mapgen.c uses this fairly extensively. Jason has 
> proposed it be used for random_map_pos(). Plus it is part of the set
> of functions for converting to and from standard coordinates.

Although I see no problem of enlarging the patch by these 3 or 4 other
one line changes: ok leave them out.

> >> k)  Macro forms for is_real_tile() and family are defined for the
> >>     cylindrical map. These will be upgraded to full topological forms
> >>     when further capabilities (i.e. topological state) are added.
> >> l)  The generic border tile macro is added and used in the current case.
> >
> >I haven't looked at these. I'm confused of the macros and functions
> >with the same name. Why?
> 

> As a user of this construct, you should be concerned with what it is 
> supposed to do, not how it does it. And this should not change if it
> does it differently down the road.
> 
> The interface is what is important. The interface should not change its
> appearance just because it is implemented as a macro or function. 
> Especially when this means updating many areas of the codebase.

Ack.

> Cosmetic 
> renames are the sign of foolish vanity or poor understanding unless there 
> is some solid and usually self-evident reasoning behind it. 
> 
> The macros will be used unless specifically undef'd. You can do this
> globally in the header file, or locally if desired. Having both forms
> preserved means that one can switch back and forth for various reasons
> and the codebase won't require any serious change.

The question is still there: why are there two forms? Do they
implement the same? Do they depend on each other?

> >> <ASIDE>
> >> Note I carefully excluded the rotational direction system from this patch, 
> >> so map.c and map.h are still significantly divergent from the corecleanups 
> >> :-).
> >
> >I'm looking forward for another thread about this topic ;)
> 
> I thought I would give you things that were relatively easy and straight
> forward to deal with on the first patch. No deep thought, no technical
> problems or serious issues, nothing really too controversial for you
> to pick on.
> 
> I'm sure the real discussions will be much more heated than this, as we
> get into real technical challenge and serious changes to Freeciv.

Take your seats and get some popcorn.

> >> Look at places like this to see what ordering they use for their direction
> >> enums. It is 2/3 of the way down.
> >> 
> >> This also shows how the coordinate system for native isometric is typically
> >> handled. It corresponds to the term "compressed native isometric" in the
> >> corecleanups.
> >> 
> >> http://www.gamedev.net/reference/articles/article747.asp
> >> </ASIDE>
> >> 
> >> To state some obvious points ...
> >> 
> >> There will probably be 20-25 patches to merge the corecleanups with CVS.
> >> There were 13 in August of which only 5 were even partially looked at, 
> >> and none of the rotational or topology sections were ever reached. It
> >> bogged down in the bugfixes and preliminaries like the current patch.
> >> Including the 3 month straightest_dir() bugfix debate.
> >
> >> I believe you recently stated that the issue with corecleanups was not
> >> getting patches. If you are waiting for these, there are probably 3-4
> >> areas that can be run in parallel, so you will get a few every week
> >> until things bog down and it becomes clear the effort is going to be as
> >> useful as the earlier one.
> >
> >Go ahead. I will try to answer every patch which gets submitted to the
> >bug tracking.
> 
> You didn't do a very good job in the past. Nor is this response/review 
> of particularly beneficial quality here. 
> 
> It is good to see you plan on trying harder, though.

I'm still a bit alone. And the new maintainer is still in his
"familiarization time".

> >> The contents of the corecleanups have been available and elements of them
> >> sometimes discussed. This is a working prototype with general documentation
> >> in the code to explain much of what is being done in various parts. This 
> >> is slightly more than a proposal tends to provide. But I assure you the
> >> read is probably far more useful. 
> >> 
> >> There is an outstanding Freeciv TODO for the cleanups and the topology, 
> >> particularly the isometric parts. Consider this as a combined submission 
> >> to deal with them.
> >
> >> I really would *like* to see discussion of various aspects thrashed out
> >> in advance, then the final patches submitted primarily for code review
> >> and sanity testing before going into CVS. It is a mistake to turn every
> >> patch into a from scratch dogfight bringing every past issue that can be
> >> tied to it into the process as part of a general ongoing guerilla war :-).
> >
> >I understand you. So if you start with a list such as above than much
> >hassle can be avoided (well at least I hope so).
> 
> Actually, this is not a list of things to be done so much as a list of
> obstacles to foul up the process. It does show the problems with the 
> current system though, quite well.
> 
> Having you micromange the process with petty requests and poor suggestions
> is not going to be particularly effective.
> 
> The decisions you should be making are whether you want to update the 
> code with certain general features, and a list of generally applicable 
> constraints that are applied consistently to all patches. When the result
> is submitted and reviewed, you can make a judgment call on any contentious
> issues, and certainly make the final pass/fail for hopefully reasonable
> and well articulated grounds.
> 
> How they get implemented will be to a large part, I hope anyway, dealt
> with by the person doing the work and people with more time, skill and 
> understanding reviewing so as to result in the best code in Freeciv.

And where and who are these people which review everything?

> >> This means discussion and requirements precede final implementation - but 
> >> this is not really part of Freeciv policy, just as significant updates or
> >> alternate devpaths are generally killed in favour of small nitfixes. It
> >> will be interesting to see how this goes :-).
> >
> >I agree with you that it is difficult.
> 
> It would be simpler if the issues were debated on technical merit and 
> not ego, ugliness, favoritism or any other of the subjective bases that
> seem to have precedence currently.
> 
> >     Raimar
> >-- 
> > email: rf13@xxxxxxxxxxxxxxxxx
> > "At the beginning of the week, we sealed ten BSD programmers
> >  into a computer room with a single distribution of BSD Unix.
> >  Upon opening the room after seven days, we found all ten programmers 
> >  dead, clutching each other's throats, and thirteen new flavors of BSD."
> 

> But as you said, the only problem is the lack of patches. 

Have I said so? (grammar error ahead) This time must had been quiet.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "SIGDANGER - The System is likely to crash soon"