GLOBAL: Bulk MSVC warning cleanup (Update) #6399

bpulliam · 2025-01-10T16:11:29Z

This PR builds on the work by elasota in #5128.

The fixes are broken up into multiple commits for clarity, but the summary is:

MULTIPLE: Add casts to prevent signed/unsigned conversion and mismatch warnings
IMGUI: Use an existing pattern to prevent redefinition of ARRAYSIZE when windows.h is included directly.
DIRECTOR,HYPNO,PRIVATE: Remove limit macros from flex skeleton to fix MSVC redefinition warnings
A fix was checked into flex to fix this issue, but it has not been included in a public release yet. It will be in 2.6.5 whenever that is released. The macros in question are not used in the skeleton and the fix prevents their inclusion. They may be removed them from the generated code with zero impact.
DIRECTOR: Fix format warning/remove MSVC-specific
In VS 2015, support was added for the %zX standard to support size_t in printf formats. The MSVC-specific code may be removed as it is no longer needed.
SAGA: Fixed a switch statement that can contain no case statements due to code inclusion/exclusion.
NANCY,COMMON: Fixed unary minus MSVC warnings
LUA: Fixed 32-bit/64-bit shift warning (MSVC)
FREESPACE: Fixed mismatch and eliminated instance where same calculation was done multiple times.

Remaining warnings (x64):
engines\tinsel\noir\spriter.cpp(376,11): warning C4146: unary minus operator applied to unsigned type, result still unsigned [...\dists\msvc\tinsel.vcxproj]
engines\tinsel\noir\spriter.cpp(380,11): warning C4146: unary minus operator applied to unsigned type, result still unsigned [...\dists\msvc\tinsel.vcxproj]
common\lua\ldo.cpp(141,3): warning C4611: interaction between '_setjmp' and C++ object destruction is non-portable [...\dists\msvc\scummvm.vcxproj]

Remaining warnings (Win32):
engines\mtropolis\boot.cpp(62,43): warning C4121: 'MTropolis::Boot::Game': alignment of a member was sensitive to packing [...\dists\msvc\mtropolis.vcxproj]
engines\tinsel\noir\spriter.cpp(376,11): warning C4146: unary minus operator applied to unsigned type, result still unsigned [...\dists\msvc\tinsel.vcxproj]
engines\tinsel\noir\spriter.cpp(380,11): warning C4146: unary minus operator applied to unsigned type, result still unsigned [...\dists\msvc\tinsel.vcxproj]
engines\twp\squirrel\sqcompiler.cpp(174,12): warning C4611: interaction between '_setjmp' and C++ object destruction is non-portable [...\dists\msvc\twp.vcxproj]
engines\twp\squirrel\sqstdrex.cpp(568,8): warning C4611: interaction between '_setjmp' and C++ object destruction is non-portable [...\dists\msvc\twp.vcxproj]
engines\twp\syslib.cpp(899,27): warning C4309: 'argument': truncation of constant value [...\dists\msvc\twp.vcxproj]
engines\ultima\ultima4\game\item.h(50,20): warning C4121: 'Ultima::Ultima4::ItemLocation': alignment of a member was sensitive to packing [...\dists\msvc\ultima.vcxproj]

… MSVC redefinition warnings

aquadran · 2025-01-10T22:32:17Z

engines/wintermute/base/gfx/base_image.cpp

@@ -97,7 +97,7 @@ bool BaseImage::loadFile(const Common::String &filename) {
 	//
 	if (_filename.hasSuffix(".tga")) {
 		_paletteTga = new byte[_paletteCount * 3];
-		for (uint32 i = 0; i < _paletteCount * 3; i += 3) {


casting should be avoided

It may be simpler to change i type to int as _paletteCount is promoted to int before being multiplied by 3.
This is guaranteed to not overflow.

lephilousophe

Thank you for the fixes.

I have a bunch of remarks because several changes are either incorrect or hide some real problems in our code (which should be properly fixed).

lephilousophe · 2025-01-12T10:42:03Z

backends/imgui/backends/imgui_impl_opengl3_loader.h

@@ -107,8 +107,24 @@ extern "C" {
 #ifndef WIN32_LEAN_AND_MEAN
 #define WIN32_LEAN_AND_MEAN 1
 #endif
+
+// HACK: Windows.h defines its own ARRAYSIZE. However, we want


ARRAYSIZE is not used in this file.
It's simpler to undefine it after the inclusion of common/system.h
Please also update the patch file.

lephilousophe · 2025-01-12T10:45:04Z

common/lua/lobject.h

@@ -356,7 +356,7 @@ typedef struct Table {
 	(check_exp((size&(size-1))==0, (cast(int, (s) & ((size)-1)))))


-#define twoto(x)	(1<<(x))
+#define twoto(x)	((size_t)1<<(x))


This does not look correct as twoto is also used with int variables.

lephilousophe · 2025-01-12T10:49:47Z

common/util.h

@@ -53,7 +53,7 @@
 #endif

 /** Template method to return the absolute value of @p x. */
-template<typename T> inline T ABS(T x)		{ return (x >= 0) ? x : -x; }
+template<typename T> inline T ABS(T x)		{ return (x >= 0) ? x : 0 - (T)x; } // 0 - (T)x is used instead of -x to silence MSVC C4146


This warning is not meant to be silenced.
Using ABS on an unsigned type is an error which should be fixed.
In fact the ABS call is just useless in this case: x will always be greater or equal to 0.

lephilousophe · 2025-01-12T10:52:48Z

engines/dgds/image.cpp

@@ -367,13 +367,13 @@ void Image::loadBitmap4(Graphics::ManagedSurface *surf, uint32 toffset, Common::
 	stream->skip(toffset >> 1);

 	if (highByte) {
-		for (uint i = 0; i < tw * th; i += 2) {
+		for (uint16 i = 0; i < tw * th; i += 2) {


This is not correct.
Multiplication of tw by th will not be limited to a 16-bits integer.
Please cast tw to uint32 in the loop condition instead.

lephilousophe · 2025-01-12T10:52:57Z

engines/dgds/image.cpp

 			byte val = stream->readByte();
 			data[i + 0] |= val & 0xF0;
 			data[i + 1] |= (val & 0x0F) << 4;
 		}
 	} else {
-		for (uint i = 0; i < tw * th; i += 2) {
+		for (uint16 i = 0; i < tw * th; i += 2) {


Same as above.

lephilousophe · 2025-01-12T11:19:03Z

backends/networking/curl/socket.cpp

@@ -123,7 +123,7 @@ bool CurlSocket::connect(const Common::String &url) {

 size_t CurlSocket::send(const char *data, int len) {
 	if (!_socket)
-		return -1;
+		return (size_t)-1;


Couldn't a u suffix silence the warning?

lephilousophe · 2025-01-12T11:19:31Z

engines/tinsel/detection.cpp

@@ -150,7 +150,7 @@ ADDetectedGame TinselMetaEngineDetection::fallbackDetect(const FileMap &allFiles
 					tmp.size = (int32)testFile.size();
 					tmp.md5 = computeStreamMD5AsString(testFile, _md5Bytes);
 				} else {
-					tmp.size = -1;
+					tmp.size = (uint)-1;


Couldn't you use a u suffix?

lephilousophe · 2025-01-12T11:22:28Z

engines/ultima/nuvie/sound/midiparser_m.cpp

@@ -164,7 +164,7 @@ bool MidiParser_M::processEvent(const EventInfo& info, bool fireEvents) {
 }

 void MidiParser_M::parseNextEvent(EventInfo &info) {
-	assert(_position._playPos - _tracks[0] < _trackLength);
+	assert((uint32)(_position._playPos - _tracks[0]) < _trackLength);


@NMIError is this subtraction always positive?

lephilousophe · 2025-01-12T11:26:37Z

engines/freescape/unpack.cpp

@@ -109,6 +109,7 @@ void unpack_data(unsigned char *unpacked_data, unsigned char *buf, unsigned int
 	unsigned char *save_buf = NULL;
 	unsigned char *save_unp = NULL;
 	unsigned int cur_unpacked_data_size = 0x00;
+	unsigned int data_left = 0x00;


The variable name seems incorrect.
This represents the data consumed: save_buf represents the start of the buffer and buf the current position.
Subtracting save_buf to buf equals to the current index of data (or the quantity consumed).

lephilousophe · 2025-01-12T11:31:34Z

engines/wintermute/base/gfx/base_image.cpp

@@ -97,7 +97,7 @@ bool BaseImage::loadFile(const Common::String &filename) {
 	//
 	if (_filename.hasSuffix(".tga")) {
 		_paletteTga = new byte[_paletteCount * 3];
-		for (uint32 i = 0; i < _paletteCount * 3; i += 3) {


It may be simpler to change i type to int as _paletteCount is promoted to int before being multiplied by 3.
This is guaranteed to not overflow.

bpulliam added 16 commits January 10, 2025 10:05

IMGUI: Fix warning for redefinition of ARRAYSIZE

174a165

DIRECTOR,HYPNO,PRIVATE: Remove limit macros from flex skeleton to fix…

8c2b044

… MSVC redefinition warnings

AGS: Fix MSVC signed/unisgned conversion warning

6cea3dc

DIRECTOR: Fix format warning/remove MSVC-specific code

c74b5a0

DIRECTOR: Fix MSVC signed/unsigned conversion warning

46e0e7b

DGDS: Fix MSVC signed/unsigned mismatch

663f425

SAGA: Fix switch statement with no case labels

6d68e47

NANCY: Fix signed/unsigned and unary minus MSVC warnings

3d08ed1

LUA: Fix 32-bit/64-bit shift warning (MSVC)

4698040

GLK: Fix MSVC signed/unsigned mismatch warning

3164609

WINTERMUTE: Fix signed/unsigned mismatch warning

a051c20

COMMON: Fix unary minus MSVC warning

c451eba

BACKENDS: Fix signed/unsigned mismatch warning

a52f406

TINSEL: Fix signed/unisgned mismatch warning

1889c6f

ULTIMA: Fix signed/unsigned mismatch

f416d0b

FREESCAPE: Fix signed/unsigned mismatch. Eliminate recalcs

8947e95

aquadran reviewed Jan 10, 2025

View reviewed changes

lephilousophe requested changes Jan 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GLOBAL: Bulk MSVC warning cleanup (Update) #6399

GLOBAL: Bulk MSVC warning cleanup (Update) #6399

bpulliam commented Jan 10, 2025 •

edited

Loading

aquadran Jan 10, 2025

lephilousophe Jan 12, 2025

lephilousophe left a comment

lephilousophe Jan 12, 2025

lephilousophe Jan 12, 2025

lephilousophe Jan 12, 2025

lephilousophe Jan 12, 2025

lephilousophe Jan 12, 2025

lephilousophe Jan 12, 2025

lephilousophe Jan 12, 2025

lephilousophe Jan 12, 2025

lephilousophe Jan 12, 2025

lephilousophe Jan 12, 2025

GLOBAL: Bulk MSVC warning cleanup (Update) #6399

Are you sure you want to change the base?

GLOBAL: Bulk MSVC warning cleanup (Update) #6399

Conversation

bpulliam commented Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lephilousophe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpulliam commented Jan 10, 2025 •

edited

Loading