Adds a new boolean option, accessiblemsg. If on, some game messages
are prefixed with direction or location information, for example:
(west): The newt bites!
(northwest): You find a hidden door.
I added the info to the most common messages, but several are
still missing it.
Pull request from entrez: in the class filtering menu for multi-drop
and for loot in-or-out of a container, make choosing 'A' without any
other filter choices (such as all, specific class(es), cursed, unpaid,
just-picked-up, &c) become a no-op.
I started with the pull request and then undid much of it. It would
have been simpler to start from scratch. If you don't have option
paranoid_confirmation:AutoAll set, when choosing 'A' for all-without-
prompting as the only selection, operate as the PR has made things
work: effectively, 'A' by itself is ignored and the operation ends
with nothing happening.
However, if you do have paranoid_confirm:A set, then continue treating
'A' by itself as if 'A'+'a': everything. Since paranoid confirmation
is specified, that will be followed by a confirmation prompt.
This also adds a context-sensitive hint to the menu about how the 'A'
entry works, shown every time when 'cmdassist' is On or just once (per
session) when that is Off.
The documentation probably needs some updating.
Closes#1143
"human", "dwarf", "elf", "gnome", and "orc" are all flagged M2_NOPOLY;
so is "giant". But dwarf and gnome are ordinary monsters and should
be eligible to be polymorph targets, so take the no-poly flag off of
them. The others are used for corpses and not intended to be distinct
monsters. But they are reasonable polymorph targets, so if player
with control picks any of them, choose a substitute. The exception is
human, which already has special poly-self handling.
src/artifact.c(1589): warning C6011: Dereferencing NULL pointer 'magr'.
The 'struct monst *magr' parameter to artifact_hit() can be Null
if 'mdef' is youmonst. mdef is nonnull.
and having temporary stoning resistance timeout before finishing.
Issue reported by Umbire: hero was able to finish eating Medusa's
corpse safely after getting the message about no longer being
protected against stoning that is given when temporary resistance
times out.
The eating code was extending temporary resistance--when eating
something protected by such--to avoid just that. I thought this
was probably a message sequencing situation but it turns out that
the code was using touch_petrifies() to test the meal. It should
use flesh_petrifies() instead; Medusa doesn't pass touch_petrifies().
I didn't figure that out until after rewriting how the duration is
extended. The old way probably would have worked as desired with
the revised petrify test but I'm checking in the new version anyway.
Fixes#1186
Two variations:
IndexOk(idx, array) validate that idx is a valid index into the array
IndexOkT(idx, array) validate that idx is a valid index into the
array, excluding the final Terminator element
Four kinds of timers are defined but only two have ever been used.
Have sanity checking complain if the other two occur or if 'kind'
doesn't match any of the four.
Also, replacing a perfectly normal use of isok() with an inline test
just to pacify static analysis feels like a slippery slope, so handle
that a little differently.
I reordered the shrink_glob timer to put all object timers together.
Unfortunately that warrants incrementing EDITLEVEL which invalidates
existing save files.
Yesterday I said that I'd done all of pager.c and part of objnam.c,
but I was talking about the prototypes in extern.h. This does more
of the same, this time for the local prototypes in pager.c so "all of
pager.c" should be accurate now.
Some functions are passed an obj or monst chain,
and the callers typically don't check them
against 0, so mark them explicitly as NO_NONNULLS
(NO_NONNULLS expands to nothing, but it flags that
some null arg analysis has been done)
Update several places where lazy lastseentyp[] might be an issue.
I think it isn't updated in a timely fashion when newsym() shows
a spot covered by an object or trap, but didn't manage to find any
cases where that caused a problem. This is more in the nature of
a precaution.
If MAKEDEFS_FILTER_NONASCII is defined (which config.h now does by
default), it will check data.base, rumors.*, and {various}.txt for
characters outside the range of ' ' through '~'. If it finds any, it
will warn about them and change them to '#'.
Tab handling is incomplete; the files that use tabs for indentation
will allow tabs anywhere, even though that's not wanted. That could
be fixed but doesn't seem particularly urgent. This is more about
spotting and repairing the special 3-char punctuation characters that
crept into data.base fairly recently.
For now, this will prevent the NONNULLxxx arguments from being
defined under a djgpp compiler or crosscompiler.
paxed reported a segfault under msdos:
nethack.exe
Exiting due to signal SIGSEGV
Page fault at eip=000c3f8c, error=0004
eax=00000000 ebx=00000000 ecx=00000000 edx=0000000f esi=00000000 edi=00000001
ebp=00589988 esp=00589970 program=C:\NH370\NETHACK.EXE
cs: sel=00a7 base=00400000 limit=0063ffff
ds: sel=00af base=00400000 limit=0063ffff
es: sel=00af base=00400000 limit=0063ffff
fs: sel=008f base=00001a20 limit=0000ffff
gs: sel=00bf base=00000000 limit=0010ffff
ss: sel=00af base=00400000 limit=0063ffff
App stack: [00589ba8..00389ba8] Exceptn stack: [00389af4..00387bb4]
Call frame traceback EIPs:
0x000c3f8c _read_config_file+19
0x0017619f _initoptions_finish+577
0x00176371 _initoptions+157
0x0025cec4 _pcmain+365
0x0025d8d9 _main+41
He was able to 'git bisect' to the macro definitions change,
and confirmed that the segfault no longer occurs after this commit.
There may be further investigation on this later.
Callgrind showed recalc_mapseen was three times more expensive (in terms
of instructions read) than anything else in our codebase. It was being
called in every vision change, re-evaluating the last seen map terrain
type for every map location in sight.
Remove updating the lastseen info in the vision code, and make a small
change so newsym() uses update_lastseentyp.
From my short tests, this seems to work correctly ...
Fix some of the extreme verbosity for null vs non-null triggered
by mklev.c. dungeon_branch() never returns Null.
'#include <assert.h>' should probably be moved out of multiple .c
files and into cstd.h or some such but this doesn't do that.
Checking the callers:
newsym() the use of see_with_infrared() is guarded by
} else if ((mon = m_at(x, y)) != 0 [...]
do_mgivenname() the use of see_with_infrared is guarded by !mtmp:
&& (!mtmp
|| (!sensemon(mtmp)
&& (!(cansee(cx, cy) || see_with_infrared(mtmp))
howmonseen(mon) dereferences mon in other places, so it would
segfault if mon were NULL; howmonseen has NONNULLARG1.
callers were checked:
domove_attackmon_at(mtmp, x, y, displaceu) has mtmp declared nonnull;
there are dereferences of mtmp in the first line of code in
the function.
In domove_core():
The 1st occurrence of is_safemon(mtmp) is guarded by if (mtmp) { }.
The 2nd occurrence of is_safemon(mtmp) is inside an if (mtmp) { } block.
The 3rd occurrence of is_safemon(mtmp) was just remediated by 987be7e8.
In lookaround():
The only occurrence of is_safemon(mtmp) is inside an
if ((mtmp = m_at(x, y)) != 0 [...] { } block.
In do_attack(mtmp), in uhitm.c:
The parameter is declared NONNULLARG1, and the 1st line of
code contains a dereference with mtmp->data, which would
segfault if mtmp were NULL.
Checking the callers:
toss_up() would have segfaulted prior to use of stone_missile() if obj were NULL.
thitu() now has a guard prior to use of stone_missile()
ohitmon() would have crashed from earlier dereference otmp->dknown if it were NULL,
otmp arg is declared nonnull
thitm() now has a guard prior to use of stone_missile().
hmon_hitmon_do_hit() null obj takes a different code path than the code path
using stone_missile(); comment asserting that added
Define some macros in include/tradstdc.h, for compilers that support
__attribute__((nonnull)), to assist in identifying which parameters
on functions are not supposed to be null pointers.
Next, for the majority of functions declared in include/extern.h, this
adds the appropriate macro that matches the actual use of each function's
parameters. The additions were done after performing some analysis.
These were the rules that were followed when determining which function
parameters should be nonnul, and which are nullable:
1. If the first use of, or reference to, the pointer parameter in the
function is a dereference, then the parameter will be considered
nonnull.
2. If there is code in the function that tests for the pointer parameter
being null, and adjusts the code-path accordingly so that no segfault
will occur, then the parameter will not be considered nonnull (it can
be null).
The use of the nonnull attributes allows the compiler to detect code in
callers of the function where a null parameter could get passed to the function.
If a warning is received the developer will have to do one of the following:
- If the null being passed to the function is now appropriate,
and the function should be able to expect a null parameter, then the
NONNULLxxx macro will have to be removed from the function's prototype.
or
- If the null being passed to the function is not appropriate,
correct the caller so it is not passing null.
or
- If the warning is about comparing to null, it may indicate an
unnecessary null check in the code involved. If it is deemed to be
unnecessary, it can then be removed.
Some static analysis tools apparently can work with the attribute, as well.
Following this, it was discovered that some functions were using one of the
(now) nonnull parameters in the first argument to the 'is_art(obj, ART)'
macro, which is defined like so:
=> #define is_art(o,art) ((o) && (o)->oartifact == (art))
That macro expansion inline resulted in a diagnostic warning because of the
'(o)' portion of the expanded macro, anywhere the macro was used with one of
the nonnull parameters. A test against null for a 'nonnull parameter' causes
a diagnostic warning.
To work around that, I replaced the is_art() macro with a function in
artifact.c, that accomplishes the same thing as the macro.
=> boolean
is_art(struct obj *obj, int art)
{
if (obj && obj->oartifact == art)
return TRUE;
return FALSE;
}
Some documentation...
These are the macros that have been defined for use when specifying the nonnull
parameters in a function prototype:
----------------------------------------------------------------------------
| Macro | Purpose |
+----------------+---------------------------------------------------------+
| NONULL | The function return value is never NULL. |
+----------------+---------------------------------------------------------+
| NONNULLPTRS | Every pointer argument is declared nonnull. |
+----------------+---------------------------------------------------------+
| NONNULLARG1 | The 1st argument is declared nonnull. |
+----------------+---------------------------------------------------------+
| NONNULLARG2 | The 2nd argument is declared nonnull. |
+----------------+---------------------------------------------------------+
| NONNULLARG3 | The 3rd argument is declared nonnull. |
+----------------+---------------------------------------------------------+
| NONNULLARG4 | The 4th argument is declared nonnull (not used). |
+----------------+---------------------------------------------------------+
| NONNULLARG5 | The 5th argument is declared nonnull. |
+----------------+---------------------------------------------------------+
| NONNULLARG7 | The 7th argument is declared nonnull (bhit). |
+----------------+---------------------------------------------------------+
| NONNULLARG12 | The 1st and 2nd arguments are declared nonnull. |
+----------------+---------------------------------------------------------+
| NONNULLARG13 | The 1st and 3rd arguments are declared nonnull. |
+----------------+---------------------------------------------------------+
| NONNULLARG123 | The 1st, 2nd and 3rd arguments are declared nonnull. |
+----------------+---------------------------------------------------------+
| NONNULLARG14 | The 1st and 4th arguments are declared nonnull. |
+----------------+---------------------------------------------------------+
| NONNULLARG134 | The 1st, 3rd and 4th arguments are declared nonnull. |
+----------------+---------------------------------------------------------+
| NONNULLARG17 | The 1st and 7th arguments are declared nonnull (this |
| | was a special-case added for askchain(), where the |
| | arguments are spread out that way. This macro |
| | could be removed if the askchain arguments in the |
| | prototype and callers were changed to make the |
| | nonnull arguments side-by-side). |
+----------------+---------------------------------------------------------+
| NONNULLARG145 | The 1st, 4th and 5th arguments are declared nonnull |
| | (this was a special-case added for find_roll_to_hit(), |
| | in uhitm.c, where the arguments are spread out that way.|
| | We can't just use NONNULLPTRS there because the 3rd |
| | argument 'weapon' can be NULL). |
+----------------+---------------------------------------------------------+
| NONNULLARG24 | The 2nd and 4th arguments are declared nonnull (this |
| | was a special-case added for query_objlist() |
| | in invent.c). |
+----------------+---------------------------------------------------------+
| NONNULLARG45 | The 4th and 5th arguments are declared nonnull (this |
| | was a special-case added for do_screen_description(), |
| | in pager.c, where the arguments are spread out that |
| | way. We can't just use NONNULLPTRS there because the |
| | 6th argument can be NULL). |
+----------------+---------------------------------------------------------+
| NO_NONNULLS | This macro expands to nothing. It is just used to |
| | mark that analysis has been done on the function, |
| | and concluded that none of the arguments could be |
| | marked nonnull.That distinguishes a function that has |
| | not been analyzed (yet), from one that has. |
+----------------+---------------------------------------------------------+
The NO_NONNULLS macro is meant to place a flag on the prototype to
make people aware that an assessed function was determined to not
be eligible for nonnull parameters. It expands to nothing.
Unfortunately, that macro was added partway through this exercise, so there
aren't many instances of it in the upper parts of include/extern.h, even though
the functions there were likely assessed and categorized as not having any
eligible nonnull parameters. It just never got any macro at all, in that case.
Following the parameter usage analysis that was done, the following was
noted:
Some NetHack functions have added a test to catch a passed null
parameter, and exit the function early as a result, or call
impossible(), and then exit. While that approach prevents segfaults
from dereferencing a null parameter, the early return is silent
(when impossible is not called anyway), and the function's true
purpose is not fulfilled. Also, the calling function may have no
awareness that the function did not complete its intended purpose,
in many instances.
Functions with such a test and early return, cannot have the parameter
declared 'nonnull', because the code to test for 'null' will cause a
diagnostic to be issued if the parameter is nonnull.
It might be good to revisit some of those functions and consider,
on a case by case basis, declaring the parameter nonnull in the
prototype, and the test/code-path commented out.
selection_getbounds() has a check and early return.
Initialization will ensure a known state if that early return
were ever taken.
This is an alternative approach to pr #1163.
The following were listed in extern.h as residing in makemon.c,
but they are actually in mon.c:
copy_mextra(struct monst *, struct monst *);
dealloc_mextra(struct monst *);
usmellmon(struct permonst *);
