In the midst of composing a commit message about how I reorganized some
of genl_putmixed()'s code without finding any problem, I realized that
there was a problem. The character immediately after \G12345678 would
be copied directly to the output buffer without examination. If that
was the leading backslash for a second encoded sequence, the G and the
hex digits would follow their backslash as just ordinary chars, which
is not what's intended. Or if instead of a backslash the next character
was the input's terminating '\0', the latter would be copied into the
output and the pointer to the input string would be incremented, then
the next loop iteraction would examined whatever followed. If valgrind
is smart enough--and it seems to be--it would complain about accessing
a character that putmixed()'s caller hadn't initialized.
The only use of putmixed() I'm sure about is the what-is code showing
a screen symbol with its explanation, which doesn't exercise either
\G12345678\G12345678 or \G12345678\0. I didn't go hunting to see if
there was someplace that might have an encoded symbol at the end of the
string. what-is still works after this patch....
The only substantive change is adding ``continue'' but I haven't gone
back and undone the reorg that preceded it.
Fix some more of the complaints from clang's static analyzer. The one
in options.c (manipulating warnings symbols) appears to be an actual bug.
All the rest are either because the analysis isn't quite sophicated
enough or outright bogus.
Two of them appear to be because a static routine is attempting to guard
against callers in the same file failing to pass in required output
pointers. Stripping away the check for missing pointer should convince
the analyzer that those output parameters always receive a value. We'll
see once the analysis is eventually re-run....
I think this should fix one of the valgrind complaints. Traps which
didn't use the trap->vl union field never initialized it, leaving a
bit of random garbage in the malloc'd trap structure. (And traps
which overwrote existing ones that did use it didn't reinitialize it
so kept stale data around.) Since those fields weren't in use by
the traps that don't care about them, this didn't provoke any actual
trouble.
Also reformatting....
This should avoid two of the three bogus clang complaints about
retaining the address of a stack variable after it has gone out of
scope.
Plus a recreation of some formatting I did a while back and then
accidentally clobbered before committing.
Fix a couple of the clang static analyzer's warnings.
muse.c has some reformatting. zap.c wasn't triggering any warning about
possible null pointer, but using MON_AT() to maybe avoid m_at() is not
a useful optimization since m_at() is a macro which starts out by using
MON_AT() itself.
When reading a novel, select a random passage which hasn't been shown
already. Once you've run through all the passages, it resets to get
them all again (with new random order that might happen to the be same
order if there aren't many passages). Switching to a different novel--
even another copy of the same one--will cause the previous passage
selection to be discarded and restarted from scratch if the prior book
is read again. Passage tracking for the most recently read novel is
kept across save and restore. (That means I needed to bump EDITLEVEL,
so it will need to be reset to 0 again before release.)
exit_nhwindows() is called before terminate(), and the tty incarnation
destroys all windows--including 'pickinv_cache_win'--without setting
the various index variables used to access them to WIN_ERR, then
terminate() calls freedynamicdata() which calls free_pickinv_cache()
which tries to destroy 'pickinv_cache_win' since it isn't WIN_ERR (if
the perm_invent option has been enabled during that playing session).
Some of the other <interface>_exit_nhwindows() also tear things down
without resetting the variables used to track them, so fixing this in
exit_nhwindows() would have been pretty messy.
Call free_pickinv_cache() before exit_nhwindows() in done(). At the
moment it's only called from done(), so other exit paths won't release
the small chunk(s) of memory used for the alternate inventory window
(if it got created for perm_invent support).
Replace the code that uses strcat with two pointers into the same buffer.
Treated separately, they point at distinct strings (no overlap possible),
but the C standard does disallow that in order to enable optimizations
using block transfer or such, so the tool that complained about it isn't
wrong. The characters getting appended to the output pointer can end
up overlapping the beginning of the other input pointer, conceivably
breaking an implementation that didn't use simple left-to-right byte-at-
a-time copying.
Also, I noticed that wishing for "luck stone" gave me a random gem.
There's code to strip off " stone" and compare against gem types, but it
prevents other code that accepts "foo bar" as a match for "foobar" and
vice versa from finding a match, since "luck" doesn't match anything
once "stone" is gone. So add the four gray stones into the array of
alternate spellings.
Another bit: it now accepts " gem" in addition to " stone" as optional
gem suffix, so "ruby", "ruby stone", and "ruby gem" all yield ruby.
("luck gem" won't work; you'll end up with a random gem-class object.)
And a last other bit: wishing for "lamp (lit) named foo" would yield
an unlit, unnamed lamp because "(lit)" followed by anything didn't match
"(lit)" and threw away everything past the opening paren. Now it will
produce "lamp named foo (lit)"--a lit lamp named "foo". (Wishing for
"lamp named foo (lit)" produces an unlit lamp named "foo (lit)". That's
acceptable to me... I'm not crawling any farther down this hole. Maybe
object formatting should be changed to keep the lit attribute in front
of the name?)
The memory leak (monst->mextra->edog, monst->mextra->mname,
monst->mextra for some monster were not released) I noticed recently
was due to recording a pet's full monster attributes with its corpse.
During save and restore, obj->oextra->omonst was being treated as a
full-fledged monster so worked as intended, but when freed, omonst
was treated as a black box and its mextra details weren't handled.
Changes to be committed:
modified: src/objects.c
modified: win/share/tilemap.c
Warnings during tile builds (and incorrect tile mappings
at run time when MAIL wasn't defined):
Creating 16x16 binary tile files (this may take some time)
warning: for tile 325 (numbered 325) of objects.txt,
found 'ETAOIN SHRDLU' while expecting 'stamped / mail'
warning: for tile 326 (numbered 326) of objects.txt,
found 'LOREM IPSUM' while expecting 'ETAOIN SHRDLU'
The recent addition of the first new extra scroll descriptions in a
very long time caused this problem to show up when MAIL was undefined.
There was a magic number in use that made an assumption that there
were only 4 such extra scroll descriptions, those being
"FOOBIE BLETCH", "TEMOV","GARVEN DEH","READ ME"
This isn't urgent, but I figure that until the mac build stuff gets
merged in, the core is still fair game....
'O' command's autopickup_exceptions was freeing a menu pick-list even
when it hadn't been allocated (for the list case, and for the remove
case if nothing was chosen for removal). That code was evidently used
as the model for msgtype and menucolors; they had the same situation.
I think ANSI and ISO sanction free(NULL) as a no-op, but pre-ANSI free
implementations don't necessarily handle that benignly. Even if they
all do, freeing something--even if that 'something' is nothing--which
hasn't been allocated is a bug on our end.
Replace several 'foo = alloc(strlen(bar)+1), strcpy(foo,bar)' sequences
with 'foo = dupstr(bar)' calls.
Change 'free(foo)' into 'free((genericptr_t) foo)' to possibly pacify
'lint' and/or really old compilers.
Add braces around 'if something;' when 'else { otherwise; }' has braces.
Simplify option value formatting for 'sortloot'.
Free sysopt.shellers and sysopt.explorers when releasing the memory used
for other sysopt fields.
Also some formatting stuff since sys.c was previously untouched.
This one has a couple of code changes included, but they shouldn't
produce any change in game play. If anyone adds a new shirt or shield
they'll have to update the corresponding foo_on() and foo_off() routines
to avoid an 'impossible' when putting on or taking off the new item(s),
the same situation as already happens for other subclasses of armor.
The automated reformatting put a space in casts of the form
'(type)(expression)', yielding '(type) (expression)', but it didn't
do that for '(typedef)(expression)'. There are lots of instances of
'(boolean)(expression)'; (uchar) and (xchar) also occur. I haven't
noticed other types, but I haven't looked in very many files yet.
Explicitly combine adjacent string literals so that pre-ANSI compilers
still have a chance to compile the code. I thought these had already
been dealt with, but I kept stumbling across them while reformatting,
so am trying to get them all out of the way now.
