#9120 closed patch
create_msvc: Updated default warnings + per-project warnings
| Reported by: | Templier | Owned by: | fingolfin |
|---|---|---|---|
| Priority: | normal | Component: | Tools |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
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.
This patch removes some of the default warnings and adds per-project warnings handling.
After the patch is applied, we are only left with 7 warnings (C4701: Potentially uninitialized local variable). Patches for most of those are attached to the tracker entry (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 (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)
Ticket imported from: #2909981. Ticket imported from: patches/1225.
Attachments (8)
Change History (35)
by , 15 years ago
| Attachment: | Build log (with C4121 and C4355 enabled).txt added |
|---|
by , 15 years ago
| Attachment: | groovie_uninitialized_variable_warnings.patch added |
|---|
Patch for groovie engine
comment:1 by , 15 years ago
Warning 4701 should be disabled, since it is too easy triggered in code, where there is no real no issue.
comment:2 by , 15 years ago
First off, thanks for this cleanup work, very much appreciated! :-)
The list of warnings to be kept seems sensible to me, most of these are not something we'd want to wrestle with. Possible exceptions:
* 4512 ('class' : assignment operator could not be generated) depending on how many of these there are, and where.
* 4996 ('function': was declared deprecated): which functions does this flag, exactly? If it flags all strcpy etc., we'll probably want to ignore it (although it *would* be good to use safer versions instead, but this is not a high priority).
* 4702 / NORETURN: We only set NORETURN for GCC and MSVC, so for any other compiler (such as MIPSpro) we don't "support" NORETURN. Also, ports may use various custom compilers we don't know, so we cannot assume in general that the compiler supports some form of NORETURN.
As for the rest, some questions:
* 4121 should be investigated
* 4189 would be nice to have (and looks easy enough to resolve), as we have a similar warning enabled for GCC, and I often end up fixing warnings about "unused variables" that people using MSVC committed. Apparently, GCC and MSVC flag a slightly different set of these, but that's not so important.
* 4355: I think we could remove the explicit "this" from those constructors -- in most (all?) cases it serves no purpose.
* 4510 / 4610: Please provide some details, where precisely do these warnings crop up? MIght indeed be possible to resolve these.
comment:3 by , 15 years ago
Kirben: 4701 only appears 7 times (most of them trivial changes), so the current code is already "clean" and maybe it's better to have it to highlight cases where there might be issues.
fingolfin: I did another run with the warnings you mention turned on and updated the build log (6581k log file with all 12355 warnings :P).
4512: there are a lot of those (10k+...), mostly coming from the common headers (the complete list will be in the build log)
4996: looks like mostly secure string&io functions as far as ScummVM is concerned. Are those even supported by all platforms? It also appears to check for deprecated POSIX names (stricmp & strdup) and those might be worth fixing. We can add _CRT_SECURE_NO_WARNINGS to silence most of the warnings until we are ready to fix the former.
4702: so it's either a warning for unreachable code or no warning although it might cause a crash on platforms with no support for NORETURN (maybe somebody can come up with some preprocessor voodoo to flag all cases where you need to handle the noreturn case :D). It seems to be handled correctly by some engines (with comments as to why the code is there). Not sure every place with an error() call is handled correctly though.
4189: I disabled it for lure only (it looks like a false positive to me). Maybe doing it locally via pragmas would be better though.
4510/4610: looks like false positives too (disabled for agi only), as those struct are initialized correctly. Manually adding constructors might be enough to resolve those though.
by , 15 years ago
| Attachment: | Build log (with C4512, C4510, C4610 and C4189 enabled).zip added |
|---|
Complete build log with more warnings turned on
comment:4 by , 15 years ago
Adding a dummy assignment operator for every class with const data fixes the warnings, but there are quite a few of them!
I added _CRT_SECURE_NO_WARNINGS to the global defines and that removes quite a lot of warnings.
The _stricmp case is trivially fixed as there is a MSVC specific define for it (behavior with VS2005 needs to checked though). For _strdup, you'll have to rename all the instances (and I'm not sure what impact it will have on other archs).
I'll upload an updated version of the create_msvc patch once you've investigated the remaining cases.
comment:5 by , 15 years ago
Some bits:
4996:
It seems MSDN talks about "stricmp" etc. being POSIX, which is not true, POSIX defines strcasecmp for that. Also "_stricmp" is no standard function either. "strdup" is no deprecated POSIX name btw. I think only MS suggests that users shouldn't use POSIX functions... If we really want MSVC to shut about about the use of "stricmp" and "strdup" we should add defines for MSVC, which defines "strdup" as "_strdup", for the use of "stricmp" we can solve this otherwise: Just check out common/scummsys.h lines 200-201, we should be just able to use "_stricmp" there instead of "stricmp". (This would be also a nice place to add the "strdup" define).
Also the warning 4996 does not really give any benefit since IIRC it suggests to use MS specific extensions instead of standard C library functions. The standard C library does feature 'secure' functions too (just think of snprintf etc.), thus this warning should be definitely disabled in my opinion.
4702:
"so it's either a warning for unreachable code or no warning although it might cause a crash on platforms with no support for NORETURN"
I don't get that sentence, NORETURN is just a hint for the compiler to know that the function will never return. It does even never return for compilers, which do not feature a similar flag, since error just calls exit (see common/textconsole.cpp line 142), which terminates the program! (Of course when you call "error" from a thread it might not have the mentioned behavior, but that's another known problem). But maybe you can enlighten me how that would now cause crashes on platforms without "NORETURN" support :-).
4121:
You can read some bits about this warning over here: http://msdn.microsoft.com/en-us/library/kabt0ka3%28VS.71%29.aspx
I wonder why it's triggered for "Kyra::ActiveWSA" at all to be honest, it should use the default compiler alignment, which should take care of alignment without any noise (at least on arches, where this is really required). It should be trivial to resolve this warning for MSVC through simple reordering of the struct members. I will commit some "fix" for that later.
comment:6 by , 15 years ago
4996: Yes, I'm fine with either disabling it globally or adding _CRT_SECURE_NO_WARNINGS to the global defines and a pair of MSVC-specific defines. In any case, the less MSVC-specific code / global changes to accommodate it, the better.
4702: Oh, I missed the exit() part being for every platform :( There is indeed no crash. But all the existing cleanup statements or other code after error() calls can be removed as it is dead code and having the warning turned on will tell us where such code is.
See for example: scumm: script_v5.cpp(752), sound.cpp(1252), midiparser_eup.cpp(156), etc.
Thing is, with C4702 turned on, I get warnings for every return call after an error, while what I would like is a warning if I'm missing one of those so I don't end up causing warnings/compilation errors on compilers without NORETURN support. Anyway, this can be disabled by default.
comment:7 by , 15 years ago
Yeah those return calls might cause warnings with C4702, but might be errors on compilers, which are not aware that "error" does not return.
comment:8 by , 15 years ago
That should read "but might avoid errors on compilers" and not "but might be..".
comment:9 by , 15 years ago
Gee, all these numbers, it's hard to keep track :)
* 4996: If this is only there to advertise proprietary replacement for strcpy & co, let's ignore it.
* 4702: As LordHoto pointed out, NORETURN is just a hint to the compiler. Let's just disable that warning.
* 4512: Now that I know what it is about, I think it's pointless -- let's disable it.
* 4189: This one is indeed a false positive -- in fact, the warning is simply *wrong*, the local variable *is* being referenced). Compiler bug?
* 4510/4610: This warning is "correct" but harmless (and I have seen it with GCC, too, by enabling some extra GCC warning options). It could be resolved but it's just not worth the effort
comment:10 by , 15 years ago
Yeah 4189 triggered in lure looks really like a compiler bug. I don't know whether we would rather want to disable it in general to avoid confusion (in case it triggers incorrectly in other places in the future too)? Or should we just limit it to lure, like this patch purposes?
comment:11 by , 15 years ago
Those secure string functions have been proposed to ISO/IEC JTC1/SC22/WG14 and they are probably going to end up in the next revision of the standard (see TR 24731-1 and http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1225.pdf). Since it's indeed not in the standard yet (and has other false positives), I've put 4996 back in the globally disabled warnings in the updated patch.
The updated patch has the following: globally disabled: 4068;4100;4103;4127;4244;4250;4310;4351;4512;4702;4706;4800;4996 agi: 4510;4610 lure: 4189
We are left with the warnings in the original build log (minus the alignment warning in kyra that lordhoto silenced yesterday).
Should we also disable 4701 as Kirben proposed?
by , 15 years ago
| Attachment: | create_msvc_disable_warnings.patch added |
|---|
Updated patch for create_msvc
comment:12 by , 15 years ago
Even if those secure string functions had made it in the standard one year ago, they'd be useless for us -- no legacy platform would support them, so using them wouldn't be portable at all.
comment:13 by , 15 years ago
Actually even if those "secure" string functions had it to the successor of C99 (they extend C99 according to the doc, which MS doesn't even implement in it's C compiler btw. ;-) and even if all our platforms supported them, I do not see any advantage of converting all of our code to use them. After all our code never had any issues from it, so it's pretty much no reason to convert it. Especially since we offer Common::String (including a printf for it, which is secure by using vsnprintf) for string handling, so in most cases converting to String would probably even offer more advantages :-).
About C4701: I don't know how reliable MSVC produces this warning, does this result in false positives? If it does I would probably vote for disabling at least in the engines, where it's triggered. After all developers can still enable them on their own, when they want to catch issues with it...
comment:14 by , 15 years ago
C4701 has resulted in several false positives in the past, which is why it should stay disabled.
comment:15 by , 15 years ago
C4701 only appears 7 times (there is an overview of the warnings in MSVC warnings table.txt). I'd rather disable it per-engine if it's ok (or just silence them since there is so few of them).
And after re-reading my older messages it looks like I'm pushing for the use of the secure string functions, which isn't the case (it was disabled in the original patch). Sorry (and I'll stop talking about it now :).
comment:16 by , 15 years ago
I applied some of the fixes for C4701. The changes in sci_uninitialized_variable_warnings.patch were incorrect, though, they changed the way the code worked (e.g. SCI_TEXT_ALIGNMENT_LEFT would have been broken had I applied the patch unchanged; the bAdd changes were wrong, too).
comment:17 by , 15 years ago
Thanks for applying those fixes. We will need someone working on SCI to help for the last warning.
Sorry about the broken SCI patch. - Did I miss something about the text alignment case? Is offset (or alignment) modified in another place and so needs to be set to 0 again? - For the bAdd case, it was very wrong indeed!
comment:18 by , 15 years ago
It seems "kDetectLofsType" is the only case, which does not setup "slc" properly. Now grepping through the code reveals that "autoDetectFeature" is only called with featureDetection == kDetectLofsType for methodNum != -1 (check 567 of the same file, it's called with an uint arugment in a loop starting from 0 on). So this warning should be rather harmless in this case, since it would never reach the branch which does use slc later on.
So probably it would be safe to initialize slc to NULL (or zero, whatever it can be initialized to ;-) to silence the warning, since it has no real effect.
comment:19 by , 15 years ago
littleboy, you are not missing anything about the alignment case, it was me who missed that "alignment" is const :).
comment:20 by , 15 years ago
Any news on that patch? The last warning to look at is C4355, every other one has been fixed in trunk already.
If nobody has time to look at it, I propose to simply disable the warning for the few projects that throw it (kyra, lure, m4 and sci) so that the original patch to create_msvc can be applied.
comment:21 by , 15 years ago
I did commit it now with C4355 for Kyra and Lure disabled, since their use looks safe. I'm not at all familiar with the m4 code, so the engine developer(s) should decide this.
by , 15 years ago
| Attachment: | create_msvc-Add_warnings_documentation.patch added |
|---|
Documented disabled warnings in cpp file
comment:22 by , 15 years ago
Added patch with disabled warnings documentation.
The only remaining warnings visible in Visual Studio are 4 C4355 in m4 (which needs review before disabling/fixing) and a single C4701 warning in mohawk. (thanks md5 for cleaning up the ones introduced in SCI recently!)
scummvm\engines\mohawk\sound.cpp(442): warning C4701: potentially uninitialized local variable 'data_chunk' used
comment:24 by , 15 years ago
Hm, in my last comment I wrote "patch committed", but didn't close this item. Is anything left to do, or can it be closed?
comment:25 by , 15 years ago
I think everything has been taken care of. ScummVM seems to build on MSVC10 (can't try with MSVC9 right now) with no warnings (the ones I mentioned in my last comment have been fixed).
comment:26 by , 15 years ago
| Owner: | set to |
|---|---|
| Status: | new → closed |
comment:27 by , 6 years ago
| Component: | → Tools |
|---|
Excerpt from build log