[Scummvm-devel] Disabled warnings in MSVC projects
Julien
scummvm at templier.info
Mon Dec 7 13:35:17 CET 2009
Hello everyone!
There was some discussion a few days ago on the IRC channel about disabled
warnings in Visual Studio. After removing all the default warnings and
looking at VS throwing 10k+ warnings, I ended up with a list of warnings
that can be disabled globally, a list of warnings that are never thrown and
a list of warnings which might be useful.
I've added a patch to the tracker that removes some of the default warnings.
I've also added code to disable specific warnings per-project.
https://sourceforge.net/tracker/?func=detail&aid=2909981&group_id=37116&atid
=418822
C4121 I'm not sure is useful, so if the people maintaining the affected
engines can take a look and tell me whether I should simply disable the
warning for their engine or if it's worth it to "fix" it in the code. Same
thing for C4355. They are disabled in the patch for the time being (an
excerpt of the build log is attached to the tracker entry if you want to
have a look at the specific lines of code affected).
After cleaning that up, we are left with only 7 warnings (C4701: Potentially
uninitialized local variable). Patches for most of those are attached to the
tracker entry and this mail (all those need review before applying!):
- Gob: draw.cpp(869): initialize matchNum to 0 (protected by bestMatch
anyway)
- Gob: hotspots.cpp(1674): initialize deltax and deltay to 0 (not sure what
was happening before)
- Groovie: cell.cpp(775): initialize result to 0 (the only branch that
doesn't sets a value has an assert)
- Sci: gui_text.cpp(389): initialize offset to 0 and remove assignement in
case SCI_TEXT_ALIGNMENT_LEFT. Not sure what the behavior was before in case
of invalid aligment.
- Sci: resource.cpp(995): initialize number to -1. processPatch already has
a check for number == -1, so bAdd might be removed too. Keeping the bAdd
check and removing the check from processPatch might also be an option if we
are sure processPatch is not going to be called from anywhere else (I'm not
sure what resources values are possible, so needs close review by a sci
engine dev.)
- Sci: state.cpp(281): no patch for this one as I have no idea what the
default value for the Selector should be (and what side-effect it might
have).
Kept the following warnings:
4068 (unknown pragma)
only used in scumm engine to mark code sections
4100 (unreferenced formal parameter)
4103 (alignment changed after including header, may be due to missing
#pragma pack(pop))
used by pack-start / pack-end
4127 (conditional expression is constant)
used in a lot of engines
4244 ('conversion' conversion from 'type1' to 'type2', possible loss of
data)
throws tons and tons of warnings, but would be nice to have it
disabled on some projects if possible
4250 (inherits 'class2::member' via dominance)
4310 (cast truncates constant value)
used in some engines, might be moved to per-engine later (there are
too many of them right now)
4351 (new behavior: elements of array 'array' will be default initialized)
a change in behavior in Visual Studio 2005. We want the new
behavior, so can be disabled
4512 ('class' : assignment operator could not be generated)
4702 (unreachable code)
mostly thrown after error() calls. Which of the supported platforms
have compilers that do not support NORETURN?
4706 (assignment within conditional expression)
used in a lot of engines
4800 ('type' : forcing value to bool 'true' or 'false' (performance
warning))
Not sure we really care about that one (and IIRC this is used in a
lot of places)
4996 ('function': was declared deprecated)
removes all the unsafe function warnings (strcpy_s, etc.)
Warnings that might be moved to per-engine (or resolved):
4121 (alignment of a member was sensitive to packing)
Draci::GPL2Function, Draci::GPL2Operator, Kyra::ActiveWSA,
Sci::SciSoundCommand are concerned.
4189 (local variable is initialized but not referenced)
in lure only and the code looks fine, so maybe adding a pragma or
disabling it only in that engine is ok
4355 ('this' : used in base member initializer list)
kyra, lure, m4 and sci are affected
4510 ('class' : default constructor could not be generated)
only agi is concerned
4610 (object 'class' can never be instantiated - user-defined constructor
required)
only agi is concerned
Removed the following warnings (never thrown):
4201 (nonstandard extension used : nameless struct/union)
4221 (nonstandard extension used : 'identifier' : cannot be initialized
using address of automatic variable)
4267 ('var' : conversion from 'size_t' to 'type', possible loss of data)
4511 ('class' : copy constructor could not be generated)
4701 (Potentially uninitialized local variable 'name' used)
Some statistics (VS2008):
- The warnings that are thrown more than a thousand times such a C4244 are
not in the table
- Some warnings might be counted twice (I tried not to count warnings in
include files or macros twice, but I surely missed some of them)
|---------------------------------------------------------------------------
---------------------------|
| Engine | C4121 | C4127 | C4189 | C4310 | C4355 | C4510 | C4610 |
C4701 | C4702 | C4706 | Total |
|---------------------------------------------------------------------------
---------------------------|
| agi | | 15 | | | | 2 | 2 |
| 1 | 11 | 31 |
| agos | | 15 | | 3 | | | |
| 5 | 4 | 27 |
| cine | | | | | | | |
| | 2 | 2 |
| cruise | | | | | | | |
| 2 | | 2 |
| draci | 2 | 5 | | | | | |
| 1 | 1 | 9 |
| drascula | | 2 | | 1 | | | |
| | 8 | 11 |
| gob | | 16 | | 14 | | | | 3
| 1 | 13 | 47 |
| groovie | | 7 | | | | | | 1
| 17 | 2 | 27 |
| kyra | 1 | 75+ | | 1 | 1 | | |
| 5 | 5 | 78+ |
| lure | | 3 | 10 | | 2 | | |
| 1 | | 16 |
| m4 | | 12 | | | | | |
| 1 | | 13 |
| made | | 3 | | 2 | 4 | | |
| 2 | 5 | 16 |
| parallaction | | 15 | | | | | |
| 1 | | 16 |
| queen | | 3 | | | | | |
| | | 3 |
| saga | | 5 | | | | | |
| 3 | 2 | 10 |
| sci | 1 | 37 | | | 1 | | | 3
| 65 | 22 | 129 |
| scumm | | 100+ | | 55 | | | |
| 53 | 7 | 233+ |
| sky | | 1 | | | | | |
| 1 | 1 | 3 |
| sword1 | | 1 | | | | | |
| 3 | | 4 |
| sword2 | | 8 | | | | | |
| 1 | 4 | 13 |
| teenagent | | 1 | | | | | |
| | | 1 |
| tinsel | | 100+ | | | | | |
| 8 | | 108+ |
| touche | | 6 | | | | | |
| | | 6 |
| tucker | | 3 | | | | | |
| | | 3 |
| scummvm | | | | 2 | | | |
| 5 | 7 | 30 |
|---------------------------------------------------------------------------
---------------------------|
| Total | 4 | 350+ | 10 | 78 | 8 | 2 | 2 | 7
| 176 | 112 | 550+ |
|---------------------------------------------------------------------------
---------------------------|
Best regards,
Julien
More information about the Scummvm-devel
mailing list