http://bugs.freeciv.org/Ticket/Display.html?id=39476 >
After others noted (PR#39450) that some iterators use indexing
instead of pointers, I promised to look into it. And spent a week
getting it to work in trunk GTK2, only to discover last night that
xaw doesn't even *compile* in trunk! And it looks like win32 doesn't
either. Folks added new parameters in client common code (and moved
unit stuff) without keeping the clients up-to-date.
So, I'm reintegrating against S2_1 instead, where I'm pretty sure
most GUI actually compile (This is a lot of extra work, I hope
you appreciate it.)
[I'll split it into several commits, as the final patch was just over
1 MB.]
===
A number of bugs became apparent. The most problematic turned out to
be a hidden unqualified "_index" (or its equivalent), used by several
of the #defined iterators. The lack of qualification prevents nesting,
either of the same or different iterators. There's been several
attempts to fix, mostly by adding more "_" "_" "___" on its front.
The easy fix is to qualify it with the pointer name: _p##_index (the
## inside a #define is a concatenator), making each iteration unique
(_p is replaced by the unique parameter).
The better fix is to eliminate the index side-effect entirely, iterate
only using pointers, and use indexing functions as needed.
For each *_iterate, I've added to the usual *_number() and *_by_number(),
as appropriate:
*_count() is the current count of items. This was often spread all over
the code, although a previous effort moved many into game.info.
*_index() is the item index. This can be calculated, instead of stored
in the actual struct. This is *only* used for arrays and bits.
*_limit() is a pointer to the last valid item, instead of the
dangerous practice of using the array size (such as &advances[A_LAST]),
a pointer *outside* the array. It will fail array index testing.
As a side benefit, these make asserts and validity testing much easier!
===
For now, *_index() is the same as *_number(), but I found considerable
inconsistency is usage of ->index: sometimes for arrays[], bit vectors,
or passed to scripting and network routines.
But the worst was the wildly multiplying, shifting, adding together,
all to make some kind of "unique" number to pass to GUI menu routines
(sometimes converting the integer to a pointer).
Therefore, my future intent is to make *_number() globally unique. The
globally unique number should be stored in the struct, and only accessed
by *_number(). This will help clean up the entire code base!
===
Once new functions were in place and #defined symbols in use, it became
more obvious that there are places where the wrong indices were used.
My favorite so far:
for(i = 0; i < 2; i++)
{
if (tech_exists(i) && advances[tech].req[i] != A_NONE)
Basically, the tech_exists(i) should either always or never work. It's
testing the index to the requirement, not the actual requirement!
It's a wonder this code runs at all
The conversion to pointers has helped enormously, as the compiler detects
the wrong type.
___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev
