Fix MSVC warnings, rename shadowed variables, fix uninitialized value…

…s, change warnings=all to use /W4.

.github/workflows/windows_builds.yml

@@ -6,7 +6,7 @@ on: [push, pull_request]
 env:
   # Only used for the cache key. Increment version to force clean build.
   GODOT_BASE_BRANCH: master
-  SCONSFLAGS: verbose=yes warnings=all werror=yes module_text_server_fb_enabled=yes
+  SCONSFLAGS: verbose=yes warnings=extra werror=yes module_text_server_fb_enabled=yes
   SCONS_CACHE_MSVC_CONFIG: true
 
 concurrency:

SConstruct

@@ -651,16 +651,32 @@ if selected_platform in platform_list:
 
     # Configure compiler warnings
     if env.msvc:  # MSVC
-        # Truncations, narrowing conversions, signed/unsigned comparisons...
-        disable_nonessential_warnings = ["/wd4267", "/wd4244", "/wd4305", "/wd4018", "/wd4800"]
-        if env["warnings"] == "extra":
-            env.Append(CCFLAGS=["/Wall"])  # Implies /W4
-        elif env["warnings"] == "all":
-            env.Append(CCFLAGS=["/W3"] + disable_nonessential_warnings)
-        elif env["warnings"] == "moderate":
-            env.Append(CCFLAGS=["/W2"] + disable_nonessential_warnings)
-        else:  # 'no'
+        if env["warnings"] == "no":
             env.Append(CCFLAGS=["/w"])
+        else:
+            if env["warnings"] == "extra":
+                env.Append(CCFLAGS=["/W4"])
+            elif env["warnings"] == "all":
+                env.Append(CCFLAGS=["/W3"])
+            elif env["warnings"] == "moderate":
+                env.Append(CCFLAGS=["/W2"])
+            # Disable warnings which we don't plan to fix.
+
+            env.Append(
+                CCFLAGS=[
+                    "/wd4100",  # C4100 (unreferenced formal parameter): Doesn't play nice with polymorphism.
+                    "/wd4127",  # C4127 (conditional expression is constant)
+                    "/wd4201",  # C4201 (non-standard nameless struct/union): Only relevant for C89.
+                    "/wd4244",  # C4244 C4245 C4267 (narrowing conversions): Unavoidable at this scale.
+                    "/wd4245",
+                    "/wd4267",
+                    "/wd4305",  # C4305 (truncation): double to float or real_t, too hard to avoid.
+                    "/wd4514",  # C4514 (unreferenced inline function has been removed)
+                    "/wd4714",  # C4714 (function marked as __forceinline not inlined)
+                    "/wd4820",  # C4820 (padding added after construct)
+                ]
+            )
+
         # Set exception handling model to avoid warnings caused by Windows system headers.
         env.Append(CCFLAGS=["/EHsc"])
 

core/config/project_settings.cpp

@@ -785,7 +785,7 @@ Error ProjectSettings::save() {
 	return error;
 }
 
-Error ProjectSettings::_save_settings_binary(const String &p_file, const RBMap<String, List<String>> &props, const CustomMap &p_custom, const String &p_custom_features) {
+Error ProjectSettings::_save_settings_binary(const String &p_file, const RBMap<String, List<String>> &p_props, const CustomMap &p_custom, const String &p_custom_features) {
 	Error err;
 	Ref<FileAccess> file = FileAccess::open(p_file, FileAccess::WRITE, &err);
 	ERR_FAIL_COND_V_MSG(err != OK, err, "Couldn't save project.binary at " + p_file + ".");
@@ -795,7 +795,7 @@ Error ProjectSettings::_save_settings_binary(const String &p_file, const RBMap<S
 
 	int count = 0;
 
-	for (const KeyValue<String, List<String>> &E : props) {
+	for (const KeyValue<String, List<String>> &E : p_props) {
 		count += E.value.size();
 	}
 
@@ -821,7 +821,7 @@ Error ProjectSettings::_save_settings_binary(const String &p_file, const RBMap<S
 		file->store_32(count); //store how many properties are saved
 	}
 
-	for (const KeyValue<String, List<String>> &E : props) {
+	for (const KeyValue<String, List<String>> &E : p_props) {
 		for (const String &key : E.value) {
 			String k = key;
 			if (!E.key.is_empty()) {
@@ -853,7 +853,7 @@ Error ProjectSettings::_save_settings_binary(const String &p_file, const RBMap<S
 	return OK;
 }
 
-Error ProjectSettings::_save_settings_text(const String &p_file, const RBMap<String, List<String>> &props, const CustomMap &p_custom, const String &p_custom_features) {
+Error ProjectSettings::_save_settings_text(const String &p_file, const RBMap<String, List<String>> &p_props, const CustomMap &p_custom, const String &p_custom_features) {
 	Error err;
 	Ref<FileAccess> file = FileAccess::open(p_file, FileAccess::WRITE, &err);
 
@@ -874,8 +874,8 @@ Error ProjectSettings::_save_settings_text(const String &p_file, const RBMap<Str
 	}
 	file->store_string("\n");
 
-	for (const KeyValue<String, List<String>> &E : props) {
-		if (E.key != props.begin()->key) {
+	for (const KeyValue<String, List<String>> &E : p_props) {
+		if (E.key != p_props.begin()->key) {
 			file->store_string("\n");
 		}
 
@@ -980,7 +980,7 @@ Error ProjectSettings::save_custom(const String &p_path, const CustomMap &p_cust
 		vclist.insert(vc);
 	}
 
-	RBMap<String, List<String>> props;
+	RBMap<String, List<String>> save_props;
 
 	for (const _VCSort &E : vclist) {
 		String category = E.name;
@@ -994,24 +994,24 @@ Error ProjectSettings::save_custom(const String &p_path, const CustomMap &p_cust
 			category = category.substr(0, div);
 			name = name.substr(div + 1, name.size());
 		}
-		props[category].push_back(name);
+		save_props[category].push_back(name);
 	}
 
-	String custom_features;
+	String save_features;
 
 	for (int i = 0; i < p_custom_features.size(); i++) {
 		if (i > 0) {
-			custom_features += ",";
+			save_features += ",";
 		}
 
 		String f = p_custom_features[i].strip_edges().replace("\"", "");
-		custom_features += f;
+		save_features += f;
 	}
 
 	if (p_path.ends_with(".godot") || p_path.ends_with("override.cfg")) {
-		return _save_settings_text(p_path, props, p_custom, custom_features);
+		return _save_settings_text(p_path, save_props, p_custom, save_features);
 	} else if (p_path.ends_with(".binary")) {
-		return _save_settings_binary(p_path, props, p_custom, custom_features);
+		return _save_settings_binary(p_path, save_props, p_custom, save_features);
 	} else {
 		ERR_FAIL_V_MSG(ERR_FILE_UNRECOGNIZED, "Unknown config file format: " + p_path + ".");
 	}

core/input/input.cpp

@@ -365,7 +365,6 @@ Vector2 Input::get_vector(const StringName &p_negative_x, const StringName &p_po
 		// Inverse lerp length to map (p_deadzone, 1) to (0, 1).
 		return vector * (Math::inverse_lerp(p_deadzone, 1.0f, length) / length);
 	}
-	return vector;
 }
 
 float Input::get_joy_axis(int p_device, JoyAxis p_axis) const {

core/input/input_event.cpp

@@ -449,11 +449,11 @@ bool InputEventKey::action_match(const Ref<InputEvent> &p_event, bool p_exact_ma
 		match &= action_mask == key_mask;
 	}
 	if (match) {
-		bool pressed = key->is_pressed();
+		bool key_pressed = key->is_pressed();
 		if (r_pressed != nullptr) {
-			*r_pressed = pressed;
+			*r_pressed = key_pressed;
 		}
-		float strength = pressed ? 1.0f : 0.0f;
+		float strength = key_pressed ? 1.0f : 0.0f;
 		if (r_strength != nullptr) {
 			*r_strength = strength;
 		}
@@ -610,20 +610,20 @@ bool InputEventMouseButton::action_match(const Ref<InputEvent> &p_event, bool p_
 	}
 
 	bool match = button_index == mb->button_index;
-	Key action_mask = get_modifiers_mask();
-	Key button_mask = mb->get_modifiers_mask();
+	Key action_modifiers_mask = get_modifiers_mask();
+	Key button_modifiers_mask = mb->get_modifiers_mask();
 	if (mb->is_pressed()) {
-		match &= (action_mask & button_mask) == action_mask;
+		match &= (action_modifiers_mask & button_modifiers_mask) == action_modifiers_mask;
 	}
 	if (p_exact_match) {
-		match &= action_mask == button_mask;
+		match &= action_modifiers_mask == button_modifiers_mask;
 	}
 	if (match) {
-		bool pressed = mb->is_pressed();
+		bool mb_pressed = mb->is_pressed();
 		if (r_pressed != nullptr) {
-			*r_pressed = pressed;
+			*r_pressed = mb_pressed;
 		}
-		float strength = pressed ? 1.0f : 0.0f;
+		float strength = mb_pressed ? 1.0f : 0.0f;
 		if (r_strength != nullptr) {
 			*r_strength = strength;
 		}
@@ -808,9 +808,9 @@ String InputEventMouseMotion::as_text() const {
 }
 
 String InputEventMouseMotion::to_string() {
-	MouseButton button_mask = get_button_mask();
-	String button_mask_string = itos((int64_t)button_mask);
-	switch (button_mask) {
+	MouseButton mouse_button_mask = get_button_mask();
+	String button_mask_string = itos((int64_t)mouse_button_mask);
+	switch (mouse_button_mask) {
 		case MouseButton::MASK_LEFT:
 			button_mask_string += vformat(" (%s)", TTRGET(_mouse_button_descriptions[(size_t)MouseButton::LEFT - 1]));
 			break;
@@ -1045,11 +1045,11 @@ bool InputEventJoypadButton::action_match(const Ref<InputEvent> &p_event, bool p
 
 	bool match = button_index == jb->button_index;
 	if (match) {
-		bool pressed = jb->is_pressed();
+		bool jb_pressed = jb->is_pressed();
 		if (r_pressed != nullptr) {
-			*r_pressed = pressed;
+			*r_pressed = jb_pressed;
 		}
-		float strength = pressed ? 1.0f : 0.0f;
+		float strength = jb_pressed ? 1.0f : 0.0f;
 		if (r_strength != nullptr) {
 			*r_strength = strength;
 		}
@@ -1340,16 +1340,16 @@ bool InputEventAction::action_match(const Ref<InputEvent> &p_event, bool p_exact
 
 	bool match = action == act->action;
 	if (match) {
-		bool pressed = act->pressed;
+		bool act_pressed = act->pressed;
 		if (r_pressed != nullptr) {
-			*r_pressed = pressed;
+			*r_pressed = act_pressed;
 		}
-		float strength = pressed ? 1.0f : 0.0f;
+		float act_strength = act_pressed ? 1.0f : 0.0f;
 		if (r_strength != nullptr) {
-			*r_strength = strength;
+			*r_strength = act_strength;
 		}
 		if (r_raw_strength != nullptr) {
-			*r_raw_strength = strength;
+			*r_raw_strength = act_strength;
 		}
 	}
 	return match;

core/io/file_access_encrypted.cpp

@@ -102,13 +102,13 @@ Error FileAccessEncrypted::open_and_parse(Ref<FileAccess> p_base, const Vector<u
 Error FileAccessEncrypted::open_and_parse_password(Ref<FileAccess> p_base, const String &p_key, Mode p_mode) {
 	String cs = p_key.md5_text();
 	ERR_FAIL_COND_V(cs.length() != 32, ERR_INVALID_PARAMETER);
-	Vector<uint8_t> key;
-	key.resize(32);
+	Vector<uint8_t> key_md5;
+	key_md5.resize(32);
 	for (int i = 0; i < 32; i++) {
-		key.write[i] = cs[i];
+		key_md5.write[i] = cs[i];
 	}
 
-	return open_and_parse(p_base, key, p_mode);
+	return open_and_parse(p_base, key_md5, p_mode);
 }
 
 Error FileAccessEncrypted::open_internal(const String &p_path, int p_mode_flags) {

core/io/file_access_network.cpp

@@ -137,12 +137,12 @@ void FileAccessNetworkClient::_thread_func() {
 				int64_t offset = get_64();
 				int32_t len = get_32();
 
-				Vector<uint8_t> block;
-				block.resize(len);
-				client->get_data(block.ptrw(), len);
+				Vector<uint8_t> resp_block;
+				resp_block.resize(len);
+				client->get_data(resp_block.ptrw(), len);
 
 				if (fa) { //may have been queued
-					fa->_set_block(offset, block);
+					fa->_set_block(offset, resp_block);
 				}
 
 			} break;

core/io/http_client_tcp.cpp

@@ -358,38 +358,38 @@ Error HTTPClientTCP::poll() {
 							} break;
 						}
 					} else if (tls) {
-						Ref<StreamPeerTLS> tls;
+						Ref<StreamPeerTLS> tls_conn;
 						if (!handshaking) {
 							// Connect the StreamPeerTLS and start handshaking.
-							tls = Ref<StreamPeerTLS>(StreamPeerTLS::create());
-							tls->set_blocking_handshake_enabled(false);
-							Error err = tls->connect_to_stream(tcp_connection, tls_verify_host, conn_host);
+							tls_conn = Ref<StreamPeerTLS>(StreamPeerTLS::create());
+							tls_conn->set_blocking_handshake_enabled(false);
+							Error err = tls_conn->connect_to_stream(tcp_connection, tls_verify_host, conn_host);
 							if (err != OK) {
 								close();
 								status = STATUS_TLS_HANDSHAKE_ERROR;
 								return ERR_CANT_CONNECT;
 							}
-							connection = tls;
+							connection = tls_conn;
 							handshaking = true;
 						} else {
 							// We are already handshaking, which means we can use your already active TLS connection.
-							tls = static_cast<Ref<StreamPeerTLS>>(connection);
-							if (tls.is_null()) {
+							tls_conn = static_cast<Ref<StreamPeerTLS>>(connection);
+							if (tls_conn.is_null()) {
 								close();
 								status = STATUS_TLS_HANDSHAKE_ERROR;
 								return ERR_CANT_CONNECT;
 							}
 
-							tls->poll(); // Try to finish the handshake.
+							tls_conn->poll(); // Try to finish the handshake.
 						}
 
-						if (tls->get_status() == StreamPeerTLS::STATUS_CONNECTED) {
+						if (tls_conn->get_status() == StreamPeerTLS::STATUS_CONNECTED) {
 							// Handshake has been successful.
 							handshaking = false;
 							ip_candidates.clear();
 							status = STATUS_CONNECTED;
 							return OK;
-						} else if (tls->get_status() != StreamPeerTLS::STATUS_HANDSHAKING) {
+						} else if (tls_conn->get_status() != StreamPeerTLS::STATUS_HANDSHAKING) {
 							// Handshake has failed.
 							close();
 							status = STATUS_TLS_HANDSHAKE_ERROR;

core/io/image.cpp

@@ -3869,15 +3869,15 @@ Dictionary Image::compute_image_metrics(const Ref<Image> p_compared_image, bool
 	double image_metric_max, image_metric_mean, image_metric_mean_squared, image_metric_root_mean_squared, image_metric_peak_snr = 0.0;
 	const bool average_component_error = true;
 
-	const uint32_t width = MIN(compared_image->get_width(), source_image->get_width());
-	const uint32_t height = MIN(compared_image->get_height(), source_image->get_height());
+	const uint32_t w = MIN(compared_image->get_width(), source_image->get_width());
+	const uint32_t h = MIN(compared_image->get_height(), source_image->get_height());
 
 	// Histogram approach originally due to Charles Bloom.
 	double hist[256];
 	memset(hist, 0, sizeof(hist));
 
-	for (uint32_t y = 0; y < height; y++) {
-		for (uint32_t x = 0; x < width; x++) {
+	for (uint32_t y = 0; y < h; y++) {
+		for (uint32_t x = 0; x < w; x++) {
 			const Color color_a = compared_image->get_pixel(x, y);
 
 			const Color color_b = source_image->get_pixel(x, y);
@@ -3922,7 +3922,7 @@ Dictionary Image::compute_image_metrics(const Ref<Image> p_compared_image, bool
 	}
 
 	// See http://richg42.blogspot.com/2016/09/how-to-compute-psnr-from-old-berkeley.html
-	double total_values = width * height;
+	double total_values = w * h;
 
 	if (average_component_error) {
 		total_values *= 4;

core/io/image_loader.cpp

@@ -51,7 +51,7 @@ bool ImageFormatLoader::recognize(const String &p_extension) const {
 }
 
 Error ImageFormatLoaderExtension::load_image(Ref<Image> p_image, Ref<FileAccess> p_fileaccess, BitField<ImageFormatLoader::LoaderFlags> p_flags, float p_scale) {
-	Error err;
+	Error err = ERR_UNAVAILABLE;
 	if (GDVIRTUAL_CALL(_load_image, p_image, p_fileaccess, p_flags, p_scale, err)) {
 		return err;
 	}