Fix remaining MSVC warnings (#6664)

VisualStudio/common.props

@@ -9,9 +9,9 @@
       <ConformanceMode>true</ConformanceMode>
       <LanguageStandard>stdcpp17</LanguageStandard>
       <WarningLevel>Level4</WarningLevel>
+      <TreatWarningAsError>true</TreatWarningAsError>
       <SDLCheck>true</SDLCheck>
       <PreprocessorDefinitions>_CRT_SECURE_NO_WARNINGS;%(PreprocessorDefinitions)</PreprocessorDefinitions>
-      <TreatSpecificWarningsAsErrors>4018;4028;4100;4114;4189;4245;4267;4309;4319;4389;4552;4456;4458;4459;4554;4457;4592;4701;4702;4706;4715;4800;4834</TreatSpecificWarningsAsErrors>
       <MultiProcessorCompilation>true</MultiProcessorCompilation>
     </ClCompile>
     <Link>

src/CMakeLists.txt

@@ -1,6 +1,6 @@
 ###########################################################################
 #   fheroes2: https://github.com/ihhub/fheroes2                           #
-#   Copyright (C) 2022                                                    #
+#   Copyright (C) 2022 - 2023                                             #
 #                                                                         #
 #   This program is free software; you can redistribute it and/or modify  #
 #   it under the terms of the GNU General Public License as published by  #
@@ -30,8 +30,6 @@ set(
 set(
 	MSVC_CC_WARN_OPTS
 	/W4
-	/wd4244 # Suppress C4244 - possible data loss during conversion
-	/wd4267 # Suppress C4267 - possible data loss during conversion from size_t
 	)
 
 add_subdirectory(thirdparty)

src/fheroes2/maps/maps_fileinfo.cpp

@@ -27,10 +27,12 @@
 #include <algorithm>
 #include <cassert>
 #include <cstring>
+#include <limits>
 #include <list>
 #include <locale>
 #include <map>
 #include <ostream>
+#include <type_traits>
 #include <utility>
 
 #include "artifact.h"
@@ -104,7 +106,7 @@ namespace
         return Color::NONE;
     }
 
-    int ByteToRace( const int byte )
+    uint8_t ByteToRace( const int byte )
     {
         switch ( byte ) {
         case 0x00:
@@ -330,7 +332,7 @@ bool Maps::FileInfo::ReadMP2( const std::string & filename )
 
     // race color
     for ( const int color : colors ) {
-        const int race = ByteToRace( fs.get() );
+        const uint8_t race = ByteToRace( fs.get() );
 
         races[Color::GetIndex( color )] = race;
 
@@ -410,14 +412,21 @@ bool Maps::FileInfo::ReadMP2( const std::string & filename )
 
 void Maps::FileInfo::FillUnions( const int side1Colors, const int side2Colors )
 {
+    using UnionsItemType = std::remove_extent_t<decltype( unions )>;
+    static_assert( std::is_same_v<UnionsItemType, uint8_t>, "Type of unions has been changed, check the logic below" );
+
     for ( int i = 0; i < KINGDOMMAX; ++i ) {
         const uint8_t color = ByteToColor( i );
 
         if ( side1Colors & color ) {
-            unions[i] = side1Colors;
+            assert( side1Colors >= std::numeric_limits<UnionsItemType>::min() && side1Colors <= std::numeric_limits<UnionsItemType>::max() );
+
+            unions[i] = static_cast<UnionsItemType>( side1Colors );
         }
         else if ( side2Colors & color ) {
-            unions[i] = side2Colors;
+            assert( side2Colors >= std::numeric_limits<UnionsItemType>::min() && side2Colors <= std::numeric_limits<UnionsItemType>::max() );
+
+            unions[i] = static_cast<UnionsItemType>( side2Colors );
         }
         else {
             unions[i] = color;

src/fheroes2/maps/maps_tiles.cpp

@@ -21,12 +21,15 @@
  *   59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.             *
  ***************************************************************************/
 
+#include "maps_tiles.h"
+
 #include <algorithm>
 #include <array>
 #include <cassert>
 #include <cstdint>
 #include <cstdlib>
 #include <iostream>
+#include <limits>
 #include <map>
 #include <set>
 #include <type_traits>
@@ -42,7 +45,6 @@
 #include "interface_gamearea.h"
 #include "logging.h"
 #include "maps.h"
-#include "maps_tiles.h"
 #include "monster.h"
 #include "monster_anim.h"
 #include "mounts.h"
@@ -776,8 +778,14 @@ Heroes * Maps::Tiles::GetHeroes() const
 void Maps::Tiles::SetHeroes( Heroes * hero )
 {
     if ( hero ) {
+        using HeroIDType = decltype( heroID );
+        static_assert( std::is_same_v<HeroIDType, uint8_t>, "Type of heroID has been changed, check the logic below" );
+
         hero->SetMapsObject( _mainObjectType );
-        heroID = hero->GetID() + 1;
+
+        assert( hero->GetID() >= std::numeric_limits<HeroIDType>::min() && hero->GetID() < std::numeric_limits<HeroIDType>::max() );
+        heroID = static_cast<HeroIDType>( hero->GetID() + 1 );
+
         SetObject( MP2::OBJ_HEROES );
     }
     else {
@@ -924,7 +932,13 @@ int Maps::Tiles::getOriginalPassability() const
 
 void Maps::Tiles::setInitialPassability()
 {
-    tilePassable = getOriginalPassability();
+    using TilePassableType = decltype( tilePassable );
+    static_assert( std::is_same_v<TilePassableType, uint16_t>, "Type of tilePassable has been changed, check the logic below" );
+
+    const int passability = getOriginalPassability();
+    assert( passability >= std::numeric_limits<TilePassableType>::min() && passability <= std::numeric_limits<TilePassableType>::max() );
+
+    tilePassable = static_cast<TilePassableType>( passability );
 }
 
 void Maps::Tiles::updatePassability()
@@ -2237,11 +2251,16 @@ void Maps::Tiles::UpdateRNDArtifactSprite( Tiles & tile )
         uidArtifact = tile._uid;
     }
 
-    updateTileById( tile, uidArtifact, art.IndexSprite() );
+    static_assert( std::is_same_v<decltype( updateTileById ), void( Tiles &, uint32_t, uint8_t )>, "Type of updateTileById() has been changed, check the logic below" );
+
+    const uint32_t artSpriteIndex = art.IndexSprite();
+    assert( artSpriteIndex > std::numeric_limits<uint8_t>::min() && artSpriteIndex <= std::numeric_limits<uint8_t>::max() );
+
+    updateTileById( tile, uidArtifact, static_cast<uint8_t>( artSpriteIndex ) );
 
     // replace artifact shadow
     if ( Maps::isValidDirection( tile._index, Direction::LEFT ) ) {
-        updateTileById( world.GetTiles( Maps::GetDirectionIndex( tile._index, Direction::LEFT ) ), uidArtifact, art.IndexSprite() - 1 );
+        updateTileById( world.GetTiles( Maps::GetDirectionIndex( tile._index, Direction::LEFT ) ), uidArtifact, static_cast<uint8_t>( artSpriteIndex - 1 ) );
     }
 }
 

src/fheroes2/maps/maps_tiles_quantity.cpp

@@ -21,9 +21,12 @@
  *   59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.             *
  ***************************************************************************/
 
+#include <cassert>
 #include <cstdint>
+#include <limits>
 #include <list>
 #include <ostream>
+#include <type_traits>
 #include <utility>
 
 #include "army_troop.h"
@@ -130,9 +133,14 @@ Skill::Secondary Maps::Tiles::QuantitySkill() const
 
 void Maps::Tiles::QuantitySetSkill( int skill )
 {
+    using Quantity1Type = decltype( quantity1 );
+    static_assert( std::is_same_v<Quantity1Type, uint8_t>, "Type of quantity1 has been changed, check the logic below" );
+
     switch ( GetObject( false ) ) {
     case MP2::OBJ_WITCHS_HUT:
-        quantity1 = skill;
+        assert( skill >= std::numeric_limits<Quantity1Type>::min() && skill <= std::numeric_limits<Quantity1Type>::max() );
+
+        quantity1 = static_cast<Quantity1Type>( skill );
         break;
 
     default:
@@ -164,13 +172,18 @@ Spell Maps::Tiles::QuantitySpell() const
 
 void Maps::Tiles::QuantitySetSpell( int spell )
 {
+    using Quantity1Type = decltype( quantity1 );
+    static_assert( std::is_same_v<Quantity1Type, uint8_t>, "Type of quantity1 has been changed, check the logic below" );
+
     switch ( GetObject( false ) ) {
     case MP2::OBJ_ARTIFACT:
     case MP2::OBJ_SHRINE_FIRST_CIRCLE:
     case MP2::OBJ_SHRINE_SECOND_CIRCLE:
     case MP2::OBJ_SHRINE_THIRD_CIRCLE:
     case MP2::OBJ_PYRAMID:
-        quantity1 = spell;
+        assert( spell >= std::numeric_limits<Quantity1Type>::min() && spell <= std::numeric_limits<Quantity1Type>::max() );
+
+        quantity1 = static_cast<Quantity1Type>( spell );
         break;
 
     default:
@@ -211,13 +224,32 @@ Artifact Maps::Tiles::QuantityArtifact() const
 
 void Maps::Tiles::QuantitySetArtifact( int art )
 {
-    quantity1 = art;
+    using Quantity1Type = decltype( quantity1 );
+    static_assert( std::is_same_v<Quantity1Type, uint8_t>, "Type of quantity1 has been changed, check the logic below" );
+
+    assert( art >= std::numeric_limits<Quantity1Type>::min() && art <= std::numeric_limits<Quantity1Type>::max() );
+
+    quantity1 = static_cast<Quantity1Type>( art );
 }
 
 void Maps::Tiles::QuantitySetResource( int res, uint32_t count )
 {
-    quantity1 = res;
-    quantity2 = res == Resource::GOLD ? count / 100 : count;
+    using Quantity1Type = decltype( quantity1 );
+    using Quantity2Type = decltype( quantity2 );
+    static_assert( std::is_same_v<Quantity1Type, uint8_t> && std::is_same_v<Quantity2Type, uint8_t>,
+                   "Type of quantity1 or quantity2 has been changed, check the logic below" );
+
+    assert( res >= std::numeric_limits<Quantity1Type>::min() && res <= std::numeric_limits<Quantity1Type>::max() );
+
+    quantity1 = static_cast<Quantity1Type>( res );
+
+    if ( res == Resource::GOLD ) {
+        count = count / 100;
+    }
+
+    assert( count >= std::numeric_limits<Quantity2Type>::min() && count <= std::numeric_limits<Quantity2Type>::max() );
+
+    quantity2 = static_cast<Quantity2Type>( count );
 }
 
 uint32_t Maps::Tiles::QuantityGold() const
@@ -352,10 +384,15 @@ Funds Maps::Tiles::QuantityFunds() const
 
 void Maps::Tiles::QuantitySetColor( int col )
 {
+    using Quantity1Type = decltype( quantity1 );
+    static_assert( std::is_same_v<Quantity1Type, uint8_t>, "Type of quantity1 has been changed, check the logic below" );
+
     switch ( GetObject( false ) ) {
     case MP2::OBJ_BARRIER:
     case MP2::OBJ_TRAVELLER_TENT:
-        quantity1 = col;
+        assert( col >= std::numeric_limits<Quantity1Type>::min() && col <= std::numeric_limits<Quantity1Type>::max() );
+
+        quantity1 = static_cast<Quantity1Type>( col );
         break;
 
     default:
@@ -951,8 +988,11 @@ uint32_t Maps::Tiles::MonsterCount() const
 
 void Maps::Tiles::MonsterSetCount( uint32_t count )
 {
+    using Quantity1Type = decltype( quantity1 );
+    static_assert( std::is_same_v<Quantity1Type, uint8_t>, "Type of quantity1 has been changed, check the logic below" );
+
     // TODO: avoid this hacky way of storing data.
-    quantity1 = count >> 8;
+    quantity1 = static_cast<Quantity1Type>( count >> 8 );
     quantity2 = 0x00FF & count;
 }
 
@@ -970,7 +1010,13 @@ void Maps::Tiles::PlaceMonsterOnTile( Tiles & tile, const Monster & mons, const
         tile._objectIcnType = MP2::OBJ_ICN_TYPE_MONS32;
     }
 
-    tile._imageIndex = mons.GetSpriteIndex();
+    using TileImageIndexType = decltype( tile._imageIndex );
+    static_assert( std::is_same_v<TileImageIndexType, uint8_t>, "Type of _imageIndex has been changed, check the logic below" );
+
+    const uint32_t monsSpriteIndex = mons.GetSpriteIndex();
+    assert( monsSpriteIndex >= std::numeric_limits<TileImageIndexType>::min() && monsSpriteIndex <= std::numeric_limits<TileImageIndexType>::max() );
+
+    tile._imageIndex = static_cast<TileImageIndexType>( monsSpriteIndex );
 
     const bool setDefinedCount = ( count > 0 );
 
@@ -1032,7 +1078,13 @@ void Maps::Tiles::UpdateMonsterInfo( Tiles & tile )
 
         // fixed random sprite
         tile.SetObject( MP2::OBJ_MONSTER );
-        tile._imageIndex = mons.GetID() - 1; // ICN::MONS32 start from PEASANT
+
+        using TileImageIndexType = decltype( tile._imageIndex );
+        static_assert( std::is_same_v<TileImageIndexType, uint8_t>, "Type of _imageIndex has been changed, check the logic below" );
+
+        assert( mons.GetID() > std::numeric_limits<TileImageIndexType>::min() && mons.GetID() <= std::numeric_limits<TileImageIndexType>::max() );
+
+        tile._imageIndex = static_cast<TileImageIndexType>( mons.GetID() - 1 ); // ICN::MONS32 starts from PEASANT
     }
 
     uint32_t count = 0;

src/thirdparty/libsmacker/smacker.c

@@ -9,6 +9,10 @@
 		Open, close, query, render, advance and seek an smk
 */
 
+#ifdef _MSC_VER
+#pragma warning( disable : 4244 ) // C4244 - possible data loss during conversion
+#endif
+
 #include "smacker.h"
 
 #include "smk_malloc.h"

src/tools/82m2wav.cpp

@@ -27,6 +27,7 @@
 #include <filesystem>
 #include <fstream> // IWYU pragma: keep
 #include <iostream>
+#include <limits>
 #include <memory>
 #include <sstream>
 #include <string>
@@ -83,7 +84,13 @@ int main( int argc, char ** argv )
             return EXIT_FAILURE;
         }
 
-        const size_t size = pos;
+        const std::make_unsigned_t<std::streamoff> posOffset = pos;
+        if ( posOffset > std::numeric_limits<size_t>::max() ) {
+            std::cerr << "File " << inputFileName << " is too large" << std::endl;
+            return EXIT_FAILURE;
+        }
+
+        const size_t size = static_cast<size_t>( posOffset );
         const auto buf = std::make_unique<char[]>( size );
 
         inputStream.seekg( 0, std::ios_base::beg );

src/tools/bin2txt.cpp

@@ -24,9 +24,11 @@
 #include <filesystem>
 #include <fstream> // IWYU pragma: keep
 #include <iostream>
+#include <limits>
 #include <sstream>
 #include <string>
 #include <system_error>
+#include <type_traits>
 #include <vector>
 
 #include "serialize.h"
@@ -82,7 +84,14 @@ int main( int argc, char ** argv )
             continue;
         }
 
-        const size_t size = pos;
+        const std::make_unsigned_t<std::streamoff> posOffset = pos;
+        if ( posOffset > std::numeric_limits<size_t>::max() ) {
+            std::cerr << "File " << inputFileName << " is too large" << std::endl;
+            // Ignore files of invalid size
+            continue;
+        }
+
+        const size_t size = static_cast<size_t>( posOffset );
         if ( size != correctBINSize ) {
             // Ignore files of invalid size
             continue;

src/tools/xmi2midi.cpp

@@ -27,6 +27,7 @@
 #include <filesystem>
 #include <fstream> // IWYU pragma: keep
 #include <iostream>
+#include <limits>
 #include <sstream>
 #include <string>
 #include <system_error>
@@ -82,7 +83,13 @@ int main( int argc, char ** argv )
             return EXIT_FAILURE;
         }
 
-        const size_t size = pos;
+        const std::make_unsigned_t<std::streamoff> posOffset = pos;
+        if ( posOffset > std::numeric_limits<size_t>::max() ) {
+            std::cerr << "File " << inputFileName << " is too large" << std::endl;
+            return EXIT_FAILURE;
+        }
+
+        const size_t size = static_cast<size_t>( posOffset );
 
         static_assert( std::is_same_v<uint8_t, unsigned char>, "uint8_t is not the same as char, check the logic below" );