Warning free builds?

[eastwind]

How cleanly does Angband build for the various targets? For VS, after fixing the four errors, there are still a bunch of warnings.

Is there interest in having changes pushed to clean those up?

Below is my VS build output showing the warnings. It's pretty much all one of two things: either a C4090 const qualifier mismatch or a C4244 for storing a 16-bit value into an 8-bit variable.

The problem with fixing the C4244 warnings is that if the 8 bit variable is being put to the save file, widening it is going to break people's saves if it gets changed to a 16-bit save-file field. So if now isn't the right time to clean this stuff up, I'd like to know before I make any changes.

If everyone is seeing equivalent errors using other compilers and just ignoring them pending a future cleanup, I can join that club. As you can see it's not a ton of warnings, but I'd say it's getting to the point of making it easy to miss a new one.

1>------ Build started: Project: Angband, Configuration: Debug Win32 ------
1>buildid.c
1>cave-map.c
1>cave-square.c
1>cave-view.c
1>cave.c
1>cmd-cave.c
1>cmd-core.c
1>cmd-misc.c
1>cmd-obj.c
1>cmd-pickup.c
1>datafile.c
1>debug.c
1>effects.c
1>game-event.c
1>game-input.c
1>game-world.c
1>gen-cave.c
1>gen-chunk.c
1>gen-monster.c
1>gen-room.c
1>Generating Code...
1>Compiling...
1>gen-util.c
1>generate.c
1>grafmode.c
1>guid.c
1>init.c
1>D:\GitHub\Eastwind921\angband\src\init.c(923,28) : warning C4090: 'function': different 'const' qualifiers
1>D:\GitHub\Eastwind921\angband\src\init.c(925,24) : warning C4090: 'function': different 'const' qualifiers
1>D:\GitHub\Eastwind921\angband\src\init.c(955,1) : warning C4244: 'initializing': conversion from 'wchar_t' to 'char', possible loss of data
1>load.c
1>main-win.c
1>message.c
1>mon-attack.c
1>mon-blows.c
1>D:\GitHub\Eastwind921\angband\src\mon-blows.c(717,45): warning C4244: '=': conversion from 's16b' to 'byte', possible loss of data
1>mon-desc.c
1>mon-group.c
1>mon-init.c
1>mon-list.c
1>mon-lore.c
1>mon-make.c
1>D:\GitHub\Eastwind921\angband\src\mon-make.c(744,37): warning C4244: '=': conversion from 's16b' to 'byte', possible loss of data
1>D:\GitHub\Eastwind921\angband\src\mon-make.c(782,36): warning C4244: '=': conversion from 's16b' to 'byte', possible loss of data
1>D:\GitHub\Eastwind921\angband\src\mon-make.c(806,36): warning C4244: '=': conversion from 's16b' to 'byte', possible loss of data
1>D:\GitHub\Eastwind921\angband\src\mon-make.c(851,36): warning C4244: '=': conversion from 's16b' to 'byte', possible loss of data
1>mon-move.c
1>mon-msg.c
1>mon-predicate.c
1>mon-spell.c
1>Generating Code...
1>Compiling...
1>mon-summon.c
1>mon-timed.c
1>mon-util.c
1>obj-chest.c
1>obj-curse.c
1>obj-desc.c
1>obj-gear.c
1>obj-ignore.c
1>obj-info.c
1>obj-init.c
1>obj-knowledge.c
1>obj-list.c
1>D:\GitHub\Eastwind921\angband\src\obj-list.c(422,45): warning C4244: '=': conversion from 'const u16b' to 'byte', possible loss of data
1>obj-make.c
1>D:\GitHub\Eastwind921\angband\src\obj-make.c(1192,41): warning C4244: '=': conversion from 's16b' to 'byte', possible loss of data
1>obj-pile.c
1>obj-power.c
1>obj-properties.c
1>obj-randart.c
1>obj-slays.c
1>obj-tval.c
1>obj-util.c
1>Generating Code...
1>Compiling...
1>option.c
1>parser.c
1>player-attack.c
1>player-birth.c
1>player-calcs.c
1>player-class.c
1>player-history.c
1>player-path.c
1>player-quest.c
1>player-race.c
1>player-spell.c
1>player-timed.c
1>player-util.c
1>player.c
1>project-feat.c
1>project-mon.c
1>project-obj.c
1>project-player.c
1>project.c
1>randname.c
1>Generating Code...
1>Compiling...
1>save.c
1>D:\GitHub\Eastwind921\angband\src\save.c(382,27) : warning C4244: 'function': conversion from 'u16b' to 'byte', possible loss of data
1>D:\GitHub\Eastwind921\angband\src\save.c(383,26) : warning C4244: 'function': conversion from 'u16b' to 'byte', possible loss of data
1>D:\GitHub\Eastwind921\angband\src\save.c(384,27) : warning C4244: 'function': conversion from 'u16b' to 'byte', possible loss of data
1>D:\GitHub\Eastwind921\angband\src\save.c(515,21) : warning C4267: 'function': conversion from 'size_t' to 'byte', possible loss of data
1>D:\GitHub\Eastwind921\angband\src\save.c(546,11) : warning C4267: 'function': conversion from 'size_t' to 'u16b', possible loss of data
1>D:\GitHub\Eastwind921\angband\src\save.c(566,11) : warning C4267: 'function': conversion from 'size_t' to 'u16b', possible loss of data
1>D:\GitHub\Eastwind921\angband\src\save.c(587,11) : warning C4267: 'function': conversion from 'size_t' to 'u16b', possible loss of data
1>D:\GitHub\Eastwind921\angband\src\save.c(592,13) : warning C4267: 'function': conversion from 'size_t' to 's16b', possible loss of data
1>savefile.c
1>score.c
1>sound-core.c
1>source.c
1>store.c
1>target.c
1>trap.c
1>ui-birth.c
1>D:\GitHub\Eastwind921\angband\src\ui-birth.c(317,37): warning C4267: 'function': conversion from 'size_t' to 'bitflag', possible loss of data
1>D:\GitHub\Eastwind921\angband\src\ui-birth.c(331,38): warning C4267: 'function': conversion from 'size_t' to 'bitflag', possible loss of data
1>D:\GitHub\Eastwind921\angband\src\ui-birth.c(413,37): warning C4267: 'function': conversion from 'size_t' to 'bitflag', possible loss of data
1>D:\GitHub\Eastwind921\angband\src\ui-birth.c(421,23): warning C4267: 'function': conversion from 'size_t' to 'bitflag', possible loss of data
1>D:\GitHub\Eastwind921\angband\src\ui-birth.c(533,23): warning C4090: 'function': different 'const' qualifiers
1>ui-command.c
1>ui-context.c
1>ui-curse.c
1>ui-death.c
1>ui-display.c
1>ui-event.c
1>ui-game.c
1>ui-help.c
1>ui-history.c
1>ui-init.c
1>ui-input.c
1>Generating Code...
1>Compiling...
1>ui-keymap.c
1>ui-knowledge.c
1>D:\GitHub\Eastwind921\angband\src\ui-knowledge.c(584,32): warning C4244: '=': conversion from 'wchar_t' to 'char', possible loss of data
1>D:\GitHub\Eastwind921\angband\src\ui-knowledge.c(600,28): warning C4244: '=': conversion from 'wchar_t' to 'byte', possible loss of data
1>D:\GitHub\Eastwind921\angband\src\ui-knowledge.c(827,44): warning C4090: 'function': different 'const' qualifiers
1>D:\GitHub\Eastwind921\angband\src\ui-knowledge.c(1065,18): warning C4090: 'function': different 'const' qualifiers
1>ui-map.c
1>ui-menu.c
1>ui-mon-list.c
1>ui-mon-lore.c
1>ui-obj-list.c
1>ui-object.c
1>ui-options.c
1>ui-output.c
1>ui-player.c
1>ui-prefs.c
1>ui-score.c
1>ui-signals.c
1>ui-spell.c
1>ui-store.c
1>ui-target.c
1>ui-term.c
1>readdib.c
1>readpng.c
1>Generating Code...
1>Compiling...
1>scrnshot.c
1>D:\GitHub\Eastwind921\angband\src\win\scrnshot.c (162,34): warning C4244: '=': conversion from 'COLORREF' to 'byte', possible loss of data
1>win-layout.c
1>wiz-debug.c
1>D:\GitHub\Eastwind921\angband\src\wiz-debug.c(601,35): warning C4244: '=': conversion from 's16b' to 'byte', possible loss of data
1>D:\GitHub\Eastwind921\angband\src\wiz-debug.c(1851,37): warning C4244: '=': conversion from 's16b' to 'byte', possible loss of data
1>wiz-spoil.c
1>wiz-stats.c
1>z-bitflag.c
1>z-color.c
1>z-dice.c
1>z-expression.c
1>D:\GitHub\Eastwind921\angband\src\z-expression.c(385,14): warning C4267: 'return': conversion from 'size_t' to 's16b', possible loss of data
1>D:\GitHub\Eastwind921\angband\src\z-expression.c(331,1): warning C4244: 'initializing': conversion from 'long' to 's16b', possible loss of data
1>z-file.c
1>z-form.c
1>z-quark.c
1>z-queue.c
1>z-rand.c
1>z-set.c
1>z-textblock.c
1>z-type.c
1>z-util.c
1>z-virt.c
1>Generating Code...
1>Angband.vcxproj -> D:\GitHub\Eastwind921\Debug\Angband.exe

[backwardsEric]

On Mac OS X (10.14 with the compiler from Xcode 11.13.1) and the source code checked out from the master branch, there are no compiler warnings for those files from running

make -f Makefile.osx

in the src directory. There is one warning for main-cocoa.m.

On Linux (Debian Buster with gcc 8.3.0) and the same source code, running

./autogen.sh ; ./confgure --with-no-install ; make

in the top-level directory also does not give any compiler warnings for those files.

I can't comment on how the choices were made for which warnings to enable or disable in src/Makefile.osx and mk/buildsys.mk.

[Pete Mack]

Some of those casts are pretty dubious looking, and should be explicit or corrected, I agree.

[eastwind]

How would you say this one should be handled:

1>mon-make.c(744,37): warning C4244: '=': conversion from 's16b' to 'byte', possible loss of data

The line in question:

obj->origin_depth = player->depth;

the warning is correct, obj->origin_depth is type byte and player->depth is type s16b

Both are saved to the save file using their respective sizes:

save.c:127: wr_byte(obj->origin_depth);
save.c:953: wr_u16b(player->depth);

So its only not causing an error because player->depth doesn't ever use its full value range (and is checked on load vs z_info->max_depth).

If I change the type of obj->origin_depth and change load.c & save.c to read & write u16b, that breaks the save file, right?

[Pete Mack]

player depth is limited to 127, so even signed char would work.

[eastwind]

Yes, in vanilla it's not causing a problem today. But it's a pothole for variants, or for vanilla should it try to increase that limit beyond 256.

I assume that once-upon-a-time all these fields were bytes, and somebody decided to increase the width of some depth fields like player->depth, but didn't do a complete job.

So now we have compiler warnings and if anybody tries to use the extra depth they'll have bugs. How should it be dealt with? Ignore the problem? Add a macro that checks & asserts if the value really does get truncated and casts otherwise? Or just fix it and break the save file?

[Derakon]

I would store player->depth (in memory) as a byte, but load it from savefiles as a s16b while asserting that it does not make use of more than 8 bits' worth of data. That seems like the best compromise to me: no broken savefiles, no compiler warnings, but a fairly well-documented workaround for a historical issue.

[eastwind]

Well, player->depth is already stored in the save file as a 16-bit, it's obj->origin_depth that's potentially too small and is the problem.

But taking your meaning,

what I could do is widen the in-memory obj->origin depth to 16 bits and continue to store it in the save as an 8-bit. The assert would then be on save, not load, the load would always work.

Having an assert on save isn't great, because you lose progress and potentially the character. But what it could do is just reduce the number silently. Since origin->depth is essentially a cosmetic value (it stores what level you found the item on but doesn't affect game play) anything you found past level 256 would be marked with the wrong level after you saved & restored.

so I could definitely solve it that way, if that's the consensus.

The thing is, this is just the first example of a problem that affects a yet-to-be-determined number of fields. You can see how many warnings there are in the OP, the next 2 size-mismatch warnings are for a different field, also in object, that one stores how many copies of the object you have. So again, things work because stacks are limited, but the code types are messed up.

[Pete Mack]

You can't have more than 40 (or 100) objects in a stack, depending on variant or version. And I can't imagine anyone wanting more than 255. I wouldn't worry about this, except to fix the in-memory number to byte.