Dev tracking for pete

[Pete Mack]

QUICKBAND

TODO:
* Get NPP 0.4.x code
* .h files.
* ui files
* gameplay files.

DONE:
* edit files
* Prepared local "quickband" git branch in NPP 0.5.2
* Fixed horrible tot_dam_aux in NPP/Quick. (Especially Quick.)
* merge edit files.

[Pete Mack]

ANGBAND:

TODO:
verify basic functionality.
re-debug bug mentioned in replay thread.

DONE:
* get & compile replay zip files.

[Pete Mack]

Mysteries in Vanilla changes

z-bitflag.h

I don't understand what is happening in flag_has. What is all the sizeof stuff? Why is it there? I can think of what might be there. But it's not what I expected.

[Pete Mack]

Quick question: is ui_event_type member redundant?

in mouse and keyboard structs, there appears to be an extra event type. Can I just duplicate the one in the containing ui_event struct for logging, or do I need to log it separately?

[Pete Mack]

The replay logging update will not be easy. I am finding cases where new characters are pushed all the way to the Term_ in the code. (This is a new change that breaks the API boundary model and thus is a breaking change for logging.) It breaks logging because if the fake keystroke is logged, it will get executed twice during replay: first the newly inserted one, and then one inserted when it was logged. I suppose the way to fix this is to disable the insertion during replay, so the logged version will get played, and the "inserted" one will never happen. This change is turning into a nightmare, because:
1. Headers are much more complex than in 3.1.v2, when I made the change initially. Just figuring out where to put extern function declarations is a chore.
2. Structures have changed.
2a. File names are all different, so just tracking down where the deltas belong is not trivial.
3. Mouse clicks are treated directly at various points in the code, so they no longer can be uniformly converted to "rich keystrokes" in util.c`inkey (or I should say ui-event.c`inkey.)

PLAN: remove all z-term.h references except the one in ui-event.c. This will control access to Term_ to the place where logging is done. (The borg works here too, so I suspect any things that break logging will likely break the borg.)
I am putting this change on hold while I instead make the code more C99 like as a familiarization task. I just discovered that my_strcmp &c are still present, which is crazy.

[takkaria]

in mouse and keyboard structs, there appears to be an extra event type. Can I just duplicate the one in the containing ui_event struct for logging, or do I need to log it separately?

You don't have to log it separately. The 'containing' ui_event is a union, not a struct, so ui_event.type and ui_event.mouse.type are the same memory location.

[Pete Mack]

I get that. But I have to use different logging code for each because I am using network-byte-order rather than native (as the savefile already does.) Although I guess this doesn't matter much anymore since essentially everyone use x86 architecture now, unless you are playing on an ARM or _really_ old OSX.
Anyway, that is the least of the problems. The tricky part is finding out what later changes that use the mouse actually require tweaking so the logger doesn't fail. It is extremely sensitive to things that work like macros--that is, things that inject/delete keystrokes into the terminal or events into the event stream (though latter shouldn't be a problem, since I *think* I made sure that all such code is bypassed by logging.)

You don't have to log it separately. The 'containing' ui_event is a union, not a struct, so ui_event.type and ui_event.mouse.type are the same memory location.

[Pete Mack]

Ansi C changes so far

Question: What does making angband ANSI compliant mean? I have done some simple cleanup. I'd like to get this over with, and submit it back to github if at all possible.

In a fresh angband master clone I have:
* got rid of my_str* functions and replaced with the standard where possible, or renamed them otherwise.
* Checked that the return codes from the "real" functions are treated properly. (One of the my_str* functions returned int where it should really return character pointer.)
* Removed all the #if standard C code from headers.
* Removed special declarations of MAX_SHORT, MAX_UCHAR.
* Changed BOOL, TRUE, FALSE to bool, true, false
* added <stdbool.h> and <stdint.h> unconditionally.

Other possible steps:
* Use "official" c types, and replace u32b with uint32_t, etc.
* Use htonl <arpa/inet.h> or <ws32.h> type stuff in the save file instead of home grown functions in savefile.c

I used the following script to actually make the changes so far, except the details in .h files and otherwise missing includes:

# NOTE: You will have to fix z-util.c and z-util.h specially.

CAT > junk.sed << EOF
s/my_stristr/stristr/g
s/my_stricmp/stricmp/g
s/my_strcat/strncat/g
s/my_strcpy/strncpy/g
s/my_strcap/strcap/g
s/my_stricmp/stricmp/g
s/strcat/strncat/g
s/TRUE/true/g
s/FALSE/false/g
s/BOOL/bool/g
s/MAX_SHORT/SHRT_MAX/g
s/MAX_UCHAR/UCHAR_MAX/g
EOF

for i in `egrep -l 'MAX_SHORT|MAX_UCHAR|strcat|my_str|TRUE|FALSE|BOOL' *.h */*.h *.c */*.c ` ;do

echo $i
sed -f junk.sed $i > $i.new 
mv -f  $i.new $i
done

rm junk.sed

[Pete Mack]

Is the checked in code in github expected to work currently? I am seeing it spin (infinite loop) immediately on character creation in a build I just downloaded from the Angband/Angband branch. "On accept character? (Y/N/c) it just hangs, with a single core pinned.

[Nick]

Is the checked in code in github expected to work currently? I am seeing it spin (infinite loop) immediately on character creation in a build I just downloaded from the Angband/Angband branch. "On accept character? (Y/N/c) it just hangs, with a single core pinned.

Is that the angband branch of angband/angband, or the master branch? because the angband branch is over 3 years old - I'm not even sure what it's doing there.

Edit: And master is working for me, after a make clean and reinstall (but not freshly downloaded).

[takkaria]

Question: What does making angband ANSI compliant mean? I have done some simple cleanup. I'd like to get this over with, and submit it back to github if at all possible.

In a fresh angband master clone I have:
* got rid of my_str* functions and replaced with the standard where possible, or renamed them otherwise.
* Checked that the return codes from the "real" functions are treated properly. (One of the my_str* functions returned int where it should really return character pointer.)
* Removed all the #if standard C code from headers.
* Removed special declarations of MAX_SHORT, MAX_UCHAR.
* Changed BOOL, TRUE, FALSE to bool, true, false
* added <stdbool.h> and <stdint.h> unconditionally.

Other possible steps:
* Use "official" c types, and replace u32b with uint32_t, etc.
* Use htonl <arpa/inet.h> or <ws32.h> type stuff in the save file instead of home grown functions in savefile.c

Some thoughts on this:

It's a mistake to replace my_strcat and my_strcpy with strncat and strncpy, since they do different things. The Angband functions are versions of the BSD strlcat and strlcpy, and just replacing them will introduce buffer overruns.

'str' is also a reserved prefix in implementations so prefixing with my_ makes sense IMO, as well as making it clearer to casual hackers what functions are defined just in Angband and what are external.

But if you want to go that route, strcasecmp is the more widely available version of my_stricmp, so it's probably worth using that and then keeping an ifdefed version for when it doesn't exist - if there is a reliable ifdef. If not, the status quo is less complex.

I think the main thing test that is needed is to make sure that MSVC++2015 can still compile the code after you make your changes. That is the compiler that has been holding us back for years. I think now it should have standard bool and int types, but there's not that much information on the web about it and I don't have a Windows set-up.

When replacing MAX_UCHAR, too, some parts of the code should probably be using UCHAR_MAX and other parts should be using UINT8_MAX, depending on whether they're using 'char' or 'byte'. MAX_SHORT should similarly be UINT16_MAX.

Also, I think the savefile code can safely keep its own byte-flipping code. It's less complex than adding more code conditional on what platform we're compiling on.

[Pete Mack]

Tak--
strncat and strncpy are ANSI C99 string functions, and they do exactly the right thing. (The only difference is strncat returns a pointer to the end of the string, not the string length.)

stricmp, however, is an ANSI extension, and it is not reliably available. The better option is to use the my_str* functions only where there's no ansi version available, but use ansi-style names and #define them to the my_str* implementation. If you look at existing code, you will see there are already uses of the ansi functions mixed in with my_str*!

Since VS 2012, basic ANSI C99 support is available for the MS compiler, so all that cruft can go away. (It's still missing some features, but it does have <stdint.h>, <stdstring.h> and <stdbool.h>)

Finally, I discovered that the infinite loop bug only exists in the optimized build OR it is intermittent. I'm not sure yet. :/

[AnonymousHero]

Finally, I discovered that the infinite loop bug only exists in the optimized build OR it is intermittent. I'm not sure yet. :/

You want to try running with clang/gcc's -fsanitize=undefined [1] to see if you're hitting undefined behavior.

[1] Or, indeed, one of the other modes to see if it's memory corruption, or...

[Pete Mack]

Finding memory corruption without purify is just massively annoying. That is just an awesome product. If anyone has a site license at work, it'd be a huge public service to do a quick run. We don't have it anymore because all our code is managed.

[hjklyubn]

Tak--
strncat and strncpy are ANSI C99 string functions, and they do exactly the right thing. (The only difference is strncat returns a pointer to the end of the string, not the string length.)

strncat does something sensible and strlcat does something sensible but those sensible things are, as takkaria said, not the same thing, even aside from the return value. The meaning of the size_t argument is quite different between the two functions. Maybe this is the cause of your infinite loop...