Angband.oook.cz
Angband.oook.cz
AboutVariantsLadderForumCompetitionComicScreenshotsFunniesLinks

Go Back   Angband Forums > Angband > Development

Reply
 
Thread Tools Display Modes
Old January 27, 2020, 03:38   #1
eastwind
Apprentice
 
Join Date: Dec 2019
Location: Mexico, undisclosed location
Posts: 79
eastwind is on a distinguished road
Warning free builds?

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
eastwind is offline   Reply With Quote
Old January 27, 2020, 04:46   #2
backwardsEric
Apprentice
 
Join Date: Aug 2019
Posts: 67
backwardsEric is on a distinguished road
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
Code:
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
Code:
./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.
backwardsEric is offline   Reply With Quote
Old January 27, 2020, 11:20   #3
Pete Mack
Prophet
 
Join Date: Apr 2007
Location: Seattle, WA
Posts: 5,736
Donated: $40
Pete Mack is on a distinguished road
Some of those casts are pretty dubious looking, and should be explicit or corrected, I agree.
Pete Mack is offline   Reply With Quote
Old January 31, 2020, 06:00   #4
eastwind
Apprentice
 
Join Date: Dec 2019
Location: Mexico, undisclosed location
Posts: 79
eastwind is on a distinguished road
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?
eastwind is offline   Reply With Quote
Old January 31, 2020, 06:09   #5
Pete Mack
Prophet
 
Join Date: Apr 2007
Location: Seattle, WA
Posts: 5,736
Donated: $40
Pete Mack is on a distinguished road
player depth is limited to 127, so even signed char would work.
Pete Mack is offline   Reply With Quote
Old January 31, 2020, 06:58   #6
eastwind
Apprentice
 
Join Date: Dec 2019
Location: Mexico, undisclosed location
Posts: 79
eastwind is on a distinguished road
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?
eastwind is offline   Reply With Quote
Old January 31, 2020, 15:07   #7
Derakon
Prophet
 
Derakon's Avatar
 
Join Date: Dec 2009
Posts: 9,023
Derakon is on a distinguished road
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.
Derakon is offline   Reply With Quote
Old January 31, 2020, 18:49   #8
eastwind
Apprentice
 
Join Date: Dec 2019
Location: Mexico, undisclosed location
Posts: 79
eastwind is on a distinguished road
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.
eastwind is offline   Reply With Quote
Old January 31, 2020, 19:11   #9
Pete Mack
Prophet
 
Join Date: Apr 2007
Location: Seattle, WA
Posts: 5,736
Donated: $40
Pete Mack is on a distinguished road
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.
Pete Mack is offline   Reply With Quote
Reply


Currently Active Users Viewing This Thread: 1 (0 members and 1 guests)
 
Thread Tools
Display Modes

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off

Forum Jump

Similar Threads
Thread Thread Starter Forum Replies Last Post
Edain builds taptap Sil 6 November 25, 2013 02:54
What are some builds you keep replaying even after winning with them? BlueFish Sil 1 October 18, 2013 22:35
Sil 1.1 Builds? chowanec Sil 7 January 12, 2013 14:22
problems with graphics- various builds Old Coach Variants 5 February 21, 2012 12:06
Question about the nightly builds Zero Vanilla 14 February 14, 2008 05:45


All times are GMT +1. The time now is 08:19.


Powered by vBulletin® Version 3.8.11
Copyright ©2000 - 2020, vBulletin Solutions Inc.