Several headers define macros that use ef_ptr() without including
"file.h". Fix that. Drop redundant inclusions elsewhere.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
... when referring to a function's parameter or a struct/union's
member.
The idea of using FOO comes from the GNU coding standards:
The comment on a function is much clearer if you use the argument
names to speak about the argument values. The variable name
itself should be lower case, but write it in upper case when you
are speaking about the value rather than the variable itself.
Thus, "the inode number NODE_NUM" rather than "an inode".
Upcasing names is problematic for a case-sensitive language like C,
because it can create ambiguity. Moreover, it's too much shouting for
my taste.
GTK-Doc's convention to prefix the identifier with @ makes references
to variables stand out nicely. The rest of the GTK-Doc conventions
make no sense for us, however.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
The cost is meant to be proportional to efficiency / 100, but the code
truncates the fraction to zero. Broken in Empire 2.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
The root cause is in put_combat(): after it sinks the ship, it calls
att_get_combat(), which treats a combat object with a dead ship as an
error, tells the attacker "not in the same sector!", and "recovers" by
putting the combat object into an error state. Too hard for me to fix
right now, so put in a FIXME comment.
The error state trips up retreat. boar() uses the victim's ship
number in the combat object to find the ship it may have to retreat.
Putting the combat object into an error state sets this number to
zero. If that ship exists, and isn't owned by the attacker, and has
RET_BOARDED set, it retreats. Oops. Broken when Empire 2 factored
out common combat code.
Fix by saving the ship number while it's still valid.
This uncovers the next bug: we now pass a dead ship to retreat_ship().
Oopses since commit f743f37. Its commit message says "Harmless, but
avoid it anyway." Going to revert.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
With offensive support but no defensive support, there's no empty line
separating the support table from the text that follows. Fix that.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
Commit cd8d742 mechanically combined struct mlist's mcp and struct
llist's llp into struct ulist's chrp, adding type casts to every use.
Not necessary, simply use mchr[] and lchr[] directly.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
lnd_put() serves two masters: march, which wants it to report
"stopped" and write back struct ulist member mobility to
unit.land.lnd_mobil, and ground combat, which doesn't.
lnd_put() assumes march when actor is non-zero. Correct (but see
commit 8c502d4). Dates back to Empire 2.
Too ugly for my taste. Specialize for each master instead:
lnd_mar_put() for march, and lnd_put() for ground combat.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
Commit d94d269 combined them into unit_put(), but that has turned out
not to be useful. Split them again.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
... get_dlist(), att_reacting_units().
This loses malloc() error checking in ask_olist() and get_dlist(). No
great loss, because we don't check in so many other places, including
att_reacting_units(). We should use a wrapper that terminates on
error, though. Left for another day.
ask_olist(), get_dlist() and att_reacting_units() zero the struct
ulist with memset(). lnd_insque() doesn't, so these functions need to
zero any members not otherwise initialized explicitly now.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
It's set to zero via memset(). Incorrect, but harmless, because it's
not actually used. Correct it anyway.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
TREATIES has issues:
* Treaties can cover attack, assault, paradrop, board, lboard, fire,
build (s|p|l|n) and enlist, but not bomb, launch, torpedo and
enlistment centers.
* Usability is very poor. While a treaty is in effect, every player
action that violates a treaty condition triggers a prompt like this:
This action is in contravention of treaty #0 (with Curmudgeon)
Do you wish to go ahead anyway? [yn]
If you decline, the action is not executed. If you accept, it is.
In both cases, your decision is reported in the news.
You cannot get rid of these prompts until the treaty expires.
* Virtually nobody uses them.
* Virtually unused code is buggy code. There is at least one race
condition: multifire() reads the firing sector, ship or land unit
before the treaty prompt, and writes it back after, triggering a
generation oops. Any updates made by other threads while trechk()
waits for input are wiped out, triggering a seqno mismatch oops.
* The treaty prompts could confuse smart clients that aren't prepared
for them. WinACE isn't, but is reported to work anyway at least
common usage. Ron Koenderink (the WinACE maintainer) suspects there
could be a few situations where it will fail.
This feature is not earning its keep. Remove it. Drop command
treaty, consider treaty, offer treaty, xdump treaty, reject treaties.
Output of accept changed, obviously.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
dist(), att_reacting_units() and s_commod() are only interested in
cost, not the actual path. BestLandPath() and BestDistPath() compute
both cost and path. Use path_find() directly instead.
Destinations are no longer treated as unreachable when the best path
is longer than 1023 characters.
Why upgrade? I'm not a lawyer, but here's my take on the differences
to version 2:
* Software patents: better protection against abuse of patents to
prevent users from exercising the rights under the GPL. I doubt
we'll get hit with a patent suit, but it's a good move just on
general principles.
* License compatibility: compatible with more free licenses, i.e. can
"steal" more free software for use in Empire. I don't expect to steal
much, but it's nice to have the option.
* Definition of "source code": modernization of some details for today's
networked world, to make it easier to distribute the software. Not
really relevant to us now, as we normally distribute full source code.
* Tivoization: this is about putting GPL-licensed software in hardware,
then make the hardware refuse to run modified software. "Neat" trick
to effectively deny its users their rights under the GPL. Abuse was
"pioneered" by TiVo (popular digital video recorders). GPLv3 forbids
it. Unlikely to become a problem for us.
* Internationalization: more careful wording, to harden the license
outside the US. The lawyers tell us it better be done that way.
* License violations: friendlier way to deal with license violations.
This has come out of past experience enforcing the GPL.
* Additional permissions: Probably not relevant to us.
Also include myself in the list of principal authors.
Land units pay a mobility penalty when marching into a non-old-owned
sector without sector mobility, to slow them down in newly taken
sectors. Attacking land units pay this penalty regardless of sector
mobility.
When attacking out of an allied sector, the penalty was computed as if
the land unit was owned by that ally. Attacking sectors old-owned by
that ally was too cheap, and taking back one's own was too expensive.
Broken since attacking land units pay the "newly taken" mobility
penalty: commit 2e693275, v4.3.6.
When an attacking sector got lost while the player was at a prompt,
and the new owner was allied to the player, the server got confused:
1. If the sector attacked with mil, the server let the ghost mil
attack, but not occupy.
2. If the sector was allied, the server reported the sector loss and
land units dropping out of the attack, but claimed the lost sector was
yours.
Fix 1. by dropping sectors from attack when they change owner away
from the player, regardless of relations. Side effect: also drops any
surviving land units there. Before, they dropped out only if the new
owner wasn't allied to the player. That change's okay.
Fix 2. the obvious way: change the messages.
Broken in 4.0.0.
SLOW_WAR has issues:
* The check whether the attacker old-owns the attacked sector is
broken, because att_abort() uses sect.sct_oldown uninitialized.
Spotted by the Clang Static Analyzer.
* Its implementation in setrel() is somewhat scary. It's actually
okay, because that part of setrel() only runs within decl(). Other
callers don't reach it: update_main() because player->god != 0
there, and the rest because they never pass a rel < HOSTILE.
* Documentation is a bit vague.
SLOW_WAR hasn't been used in a public game in years. Fixing it is not
worth it, so remove it instead.
It passed def->own to lnd_sweep(), which looks like a bug. But it's
actually player->cnum there, because take_def() already set def->own
to player->owner: take_def() first changes the owner of the attacked
sector by calling takeover(), then updates def->own from that in
att_get_combat().
take_def() and ask_move_in() printed both to the current player and to
land unit owner. Their use of prcom() and xyas() looked particularly
suspicious: they used the current player, then printed the result to
the land unit owner. Fortunately, current player and land unit owner
are the same, since even even deities can't attack with foreign land
units. Normalize to current player for consistency.
Switch get_ototal(), get_oland(), kill_land() and move_in_land() to
current player as well.
getland() is obviously redundant in get_dlist(), because the land unit
can't have changed since the previous read.
What's up with ask_olist() is less obvious. For each capable land
unit, ask_olist() asks the player whether to attack with it. If it
attacks, it gets copied into olist. Since asking the player yields
the processor, these copies can become stale. However, re-reading
fixes that just for the last one. Moreover, the code copes with stale
copies just fine: ask_olist() is always directly followed by
att_get_offense() or att_move_in_off(), and both replace the
potentially stale copy in olist.
ask_olist() and get_dlist() called get_land() with llp->chrp still
null, which made it initialize parts of llp instead of performing its
usual integrity check. att_reacting_units() had the initialization
inline. Change the other two to match, and simplify get_land(). No
functional change.
Seamines and landmines share storage. Sea and bridge span sectors can
hold only sea mines, other sector types only landmines. Sector type
checks were missing or incorrect in several places:
* Seamines under bridge spans were mistaken for landmines in several
places:
- ground combat mine defense bonus, in get_mine_dsupport() and
stre(),
- land units retreating from bombs, in retreat_land1(),
- non-land unit ground movement (commands explore, move, transport,
and INTERDICT_ATT of military), in check_lmines(),
Fix them to check the sector type with new SCT_MINES_ARE_SEAMINES(),
SCT_LANDMINES().
* plane_sweep() mistook landmines for seamines in harbors. Bug could
not bite, because it's only called for sea sectors. Drop the bogus
check for harbor.
* Collapsing a bridge tower magically converted landmines into
seamines. Make knockdown() clear landmines.
Also use SCT_MINES_ARE_SEAMINES() and SCT_LANDMINES() in mine(),
landmine(), lnd_sweep() and lnd_check_mines(). No functional change
there.
Keep checking only for sea in pln_mine(), plane_sweep(),
retreat_ship1(), shp_sweep() and shp_check_one_mines(). This means
seamines continue not to work under bridges. Making them work there
is tempting, but as long as finding seamines clobbers the sector
designation in the bmap, it's better to have them in sea sectors only.
Historical notes:
Mines started out simple enough: you could mine sea and bridge spans,
and ships hit and swept mines in foreign sectors.
Chainsaw 2 introduced aerial mining and sweeping. Unlike ships,
planes could not mine bridge spans. plane_sweep() could sweep
harbors, which was wrong, but it was never called there, so the bug
could not bite.
Chainsaw 3 introduced landmines. The idea was to permit only seamines
in some sector types, and only landmines in the others, so they can
share storage. To figure out whether a sector has a particular kind
of mines, you need to check the sector type. Such checks already
existed in mine, drop and sweep, and they were kept unchanged. The
new lmine command also got the check. Everything else did not.
Ground movement and combat could hit and sweep seamines in bridge
spans. Ships could hit and sweep landmines in harbors.
Empire 2 fixed land unit movement (march, INTERDICT_ATT) not to
mistake seamines for landmines on bridge spans. It fixed ships not to
mistake landmines for seamines. The fix also neutered seamines under
bridge spans: ships could neither hit nor sweep them anymore. Both
fixes missed retreat.
Commit 5663713b (v4.3.1) made ship retreat consistent with other ship
movement.
Pinpointed assignments within if conditionals with spatch -sp_file
tests/bad_assign.cocci (from coccinelle-0.1.4). Cherry-picked diff
hunks affecting conditionals split over multiple lines, and cleaned
them up.
