feat(CI): add cppcheck (#15211)

Co-authored-by: Skjalf <47818697+Nyeriah@users.noreply.github.com>

.github/workflows/cpp-check.yml

@@ -0,0 +1,36 @@
+name: cpp-check
+on:
+  push:
+    branches:
+      - "master"
+    paths:
+      - src/**
+      - "!README.md"
+      - "!docs/**"
+  pull_request:
+    paths:
+      - src/**
+      - "!README.md"
+      - "!docs/**"
+
+jobs:
+  cpp-check:
+    strategy:
+      fail-fast: false
+    runs-on: ubuntu-22.04
+    name: cpp check
+    steps:
+      - uses: actions/checkout@v2
+      - name: cpp check
+        run: |
+          sudo apt update -y
+          sudo apt install -y cppcheck
+          cppcheck --force --inline-suppr \
+            -i src/server/game/Achievements/AchievementMgr.cpp \
+            -i src/server/game/AuctionHouse/AuctionHouseMgr.cpp \
+            -i src/server/game/Battlegrounds/Zones/BattlegroundSA.cpp \
+            -i src/server/game/DungeonFinding/LFGMgr.cpp \
+            -i src/server/game/Entities/GameObject/GameObject.cpp \
+            -i src/server/game/Entities/Pet/Pet.cpp \
+            -i src/server/game/Entities/Player/Player.cpp \
+            src/

src/common/Collision/BoundingIntervalHierarchy.cpp

@@ -26,6 +26,7 @@
 void BIH::buildHierarchy(std::vector<uint32>& tempTree, buildData& dat, BuildStats& stats)
 {
     // create space for the first node
+    // cppcheck-suppress integerOverflow
     tempTree.push_back(uint32(3 << 30)); // dummy leaf
     tempTree.insert(tempTree.end(), 2, 0);
     //tempTree.add(0);

src/common/Collision/BoundingIntervalHierarchy.h

@@ -187,9 +187,9 @@ public:
             while (true)
             {
                 uint32 tn = tree[node];
-                uint32 axis = (tn & (3 << 30)) >> 30;
+                uint32 axis = (tn & (3 << 30)) >> 30; // cppcheck-suppress integerOverflow
-                bool BVH2 = tn & (1 << 29);
+                bool BVH2 = tn & (1 << 29); // cppcheck-suppress integerOverflow
-                int offset = tn & ~(7 << 29);
+                int offset = tn & ~(7 << 29); // cppcheck-suppress integerOverflow
                 if (!BVH2)
                 {
                     if (axis < 3)
@@ -297,9 +297,9 @@ public:
             while (true)
             {
                 uint32 tn = tree[node];
-                uint32 axis = (tn & (3 << 30)) >> 30;
+                uint32 axis = (tn & (3 << 30)) >> 30; // cppcheck-suppress integerOverflow
-                bool BVH2 = tn & (1 << 29);
+                bool BVH2 = tn & (1 << 29); // cppcheck-suppress integerOverflow
-                int offset = tn & ~(7 << 29);
+                int offset = tn & ~(7 << 29); // cppcheck-suppress integerOverflow
                 if (!BVH2)
                 {
                     if (axis < 3)
@@ -425,7 +425,7 @@ protected:
     void createNode(std::vector<uint32>& tempTree, int nodeIndex, uint32 left, uint32 right) const
     {
         // write leaf node
-        tempTree[nodeIndex + 0] = (3 << 30) | left;
+        tempTree[nodeIndex + 0] = (3 << 30) | left; // cppcheck-suppress integerOverflow
         tempTree[nodeIndex + 1] = right - left + 1;
     }
 

src/common/Platform/ServiceWin32.cpp

@@ -42,6 +42,7 @@ SERVICE_STATUS serviceStatus;
 
 SERVICE_STATUS_HANDLE serviceStatusHandle = 0;
 
+// cppcheck-suppress syntaxError
 typedef WINADVAPI BOOL (WINAPI* CSD_T)(SC_HANDLE, DWORD, LPCVOID);
 
 bool WinServiceInstall()

src/server/database/Database/Transaction.cpp

@@ -110,7 +110,7 @@ bool TransactionTask::Execute()
             // Make sure only 1 async thread retries a transaction so they don't keep dead-locking each other
             std::lock_guard<std::mutex> lock(_deadlockLock);
 
-            for (Milliseconds loopDuration = 0s, startMSTime = GetTimeMS(); loopDuration <= DEADLOCK_MAX_RETRY_TIME_MS; loopDuration = GetMSTimeDiffToNow(startMSTime))
+            for (Milliseconds loopDuration{}, startMSTime = GetTimeMS(); loopDuration <= DEADLOCK_MAX_RETRY_TIME_MS; loopDuration = GetMSTimeDiffToNow(startMSTime))
             {
                 if (!TryExecute())
                     return true;
@@ -157,7 +157,7 @@ bool TransactionWithResultTask::Execute()
             // Make sure only 1 async thread retries a transaction so they don't keep dead-locking each other
             std::lock_guard<std::mutex> lock(_deadlockLock);
 
-            for (Milliseconds loopDuration = 0s, startMSTime = GetTimeMS(); loopDuration <= DEADLOCK_MAX_RETRY_TIME_MS; loopDuration = GetMSTimeDiffToNow(startMSTime))
+            for (Milliseconds loopDuration{}, startMSTime = GetTimeMS(); loopDuration <= DEADLOCK_MAX_RETRY_TIME_MS; loopDuration = GetMSTimeDiffToNow(startMSTime))
             {
                 if (!TryExecute())
                 {

src/server/game/Autobroadcast/AutobroadcastMgr.cpp

@@ -129,12 +129,12 @@ void AutobroadcastMgr::SendAutobroadcasts()
     LOG_DEBUG("autobroadcast", "AutobroadcastMgr::SendAutobroadcasts: '{}'", msg);
 }
 
-void AutobroadcastMgr::SendWorldAnnouncement(std::string_view msg)
+void AutobroadcastMgr::SendWorldAnnouncement(std::string msg)
 {
     sWorld->SendWorldTextOptional(LANG_AUTO_BROADCAST, ANNOUNCER_FLAG_DISABLE_AUTOBROADCAST, msg.data());
 }
 
-void AutobroadcastMgr::SendNotificationAnnouncement(std::string_view msg)
+void AutobroadcastMgr::SendNotificationAnnouncement(std::string msg)
 {
     WorldPacket data(SMSG_NOTIFICATION, (msg.size() + 1));
     data << msg.data();

src/server/game/Autobroadcast/AutobroadcastMgr.h

@@ -37,8 +37,8 @@ public:
     void SendAutobroadcasts();
 
 private:
-    void SendWorldAnnouncement(std::string_view msg);
+    void SendWorldAnnouncement(std::string msg);
-    void SendNotificationAnnouncement(std::string_view msg);
+    void SendNotificationAnnouncement(std::string msg);
 
     typedef std::map<uint8, std::string> AutobroadcastsMap;
     typedef std::map<uint8, uint8> AutobroadcastsWeightMap;

src/server/game/Battlegrounds/BattlegroundQueue.cpp

@@ -895,8 +895,8 @@ void BattlegroundQueue::BattlegroundQueueUpdate(uint32 diff, BattlegroundTypeId
             {
                 if (!(*itr3)->IsInvitedToBGInstanceGUID
                     && (((*itr3)->ArenaMatchmakerRating >= arenaMinRating && (*itr3)->ArenaMatchmakerRating <= arenaMaxRating) || (int32)(*itr3)->JoinTime < discardTime)
-                    && ((*itr_teams[0])->ArenaTeamId != (*itr3)->PreviousOpponentsTeamId || ((int32)(*itr3)->JoinTime < discardOpponentsTime))
+                    && ((*(itr_teams[0]))->ArenaTeamId != (*itr3)->PreviousOpponentsTeamId || ((int32)(*itr3)->JoinTime < discardOpponentsTime))
-                    && (*itr_teams[0])->ArenaTeamId != (*itr3)->ArenaTeamId)
+                    && (*(itr_teams[0]))->ArenaTeamId != (*itr3)->ArenaTeamId)
                 {
                     itr_teams[found++] = itr3;
                     break;
@@ -907,8 +907,8 @@ void BattlegroundQueue::BattlegroundQueueUpdate(uint32 diff, BattlegroundTypeId
         //if we have 2 teams, then start new arena and invite players!
         if (found == 2)
         {
-            GroupQueueInfo* aTeam = *itr_teams[TEAM_ALLIANCE];
+            GroupQueueInfo* aTeam = *(itr_teams[TEAM_ALLIANCE]);
-            GroupQueueInfo* hTeam = *itr_teams[TEAM_HORDE];
+            GroupQueueInfo* hTeam = *(itr_teams[TEAM_HORDE]);
 
             Battleground* arena = sBattlegroundMgr->CreateNewBattleground(bgTypeId, bracketEntry, arenaType, true);
             if (!arena)

src/server/game/Entities/Player/PlayerQuest.cpp

@@ -1310,6 +1310,7 @@ bool Player::SatisfyQuestSeasonal(Quest const* qInfo, bool /*msg*/) const
     if (!qInfo->IsSeasonal() || m_seasonalquests.empty())
         return true;
 
+    // cppcheck-suppress mismatchingContainers
     Player::SeasonalEventQuestMap::iterator itr = ((Player*)this)->m_seasonalquests.find(qInfo->GetEventIdForQuest());
 
     if (itr == m_seasonalquests.end() || itr->second.empty())

src/server/game/Loot/LootMgr.cpp

@@ -190,6 +190,7 @@ uint32 LootStore::LoadLootTable()
 
         // Looking for the template of the entry
         // often entries are put together
+        // cppcheck-suppress eraseDereference
         if (m_LootTemplates.empty() || tab->first != entry)
         {
             // Searching the template (in case template Id changed)

src/server/scripts/EasternKingdoms/BlackrockMountain/BlackrockDepths/blackrock_depths.cpp

@@ -121,13 +121,13 @@ public:
     };
 };
 
-struct Wave
+struct WaveCreature
 {
     uint32 entry;
     uint32 amount;
 };
 
-static Wave RingMobs[] = // different amounts based on the type
+static WaveCreature RingMobs[] = // different amounts based on the type
 {
     {NPC_DREDGE_WORM, 3},
     {NPC_DEEP_STINGER, 3},

src/server/scripts/EasternKingdoms/zone_hinterlands.cpp

@@ -49,18 +49,18 @@ enum Rinji
     GO_RINJI_CAGE           = 142036
 };
 
-struct Location
+struct LocationXYZ
 {
-    float posX, posY, posZ;
+    float x, y, z;
 };
 
-Location AmbushSpawn[] =
+LocationXYZ AmbushSpawn[] =
 {
     { 191.296204f, -2839.329346f, 107.388f },
     { 70.972466f,  -2848.674805f, 109.459f }
 };
 
-Location AmbushMoveTo[] =
+LocationXYZ AmbushMoveTo[] =
 {
     { 166.630386f, -2824.780273f, 108.153f },
     { 70.886589f,  -2874.335449f, 116.675f }
@@ -118,12 +118,12 @@ public:
             if (!_first)
                 spawnId = 1;
 
-            me->SummonCreature(NPC_RANGER, AmbushSpawn[spawnId].posX, AmbushSpawn[spawnId].posY, AmbushSpawn[spawnId].posZ, 0.0f,
+            me->SummonCreature(NPC_RANGER, AmbushSpawn[spawnId].x, AmbushSpawn[spawnId].y, AmbushSpawn[spawnId].z, 0.0f,
                                TEMPSUMMON_TIMED_OR_CORPSE_DESPAWN, 60000);
 
             for (int i = 0; i < 2; ++i)
             {
-                me->SummonCreature(NPC_OUTRUNNER, AmbushSpawn[spawnId].posX, AmbushSpawn[spawnId].posY, AmbushSpawn[spawnId].posZ, 0.0f,
+                me->SummonCreature(NPC_OUTRUNNER, AmbushSpawn[spawnId].x, AmbushSpawn[spawnId].y, AmbushSpawn[spawnId].z, 0.0f,
                                    TEMPSUMMON_TIMED_OR_CORPSE_DESPAWN, 60000);
             }
         }
@@ -131,7 +131,7 @@ public:
         void JustSummoned(Creature* summoned) override
         {
             summoned->SetWalk(false);
-            summoned->GetMotionMaster()->MovePoint(0, AmbushMoveTo[spawnId].posX, AmbushMoveTo[spawnId].posY, AmbushMoveTo[spawnId].posZ);
+            summoned->GetMotionMaster()->MovePoint(0, AmbushMoveTo[spawnId].x, AmbushMoveTo[spawnId].y, AmbushMoveTo[spawnId].z);
         }
 
         void sQuestAccept(Player* player, Quest const* quest) override

src/server/scripts/EasternKingdoms/zone_undercity.cpp

@@ -748,12 +748,11 @@ enum Worldstates
     WORLD_STATE_FAIL_H = 3878
 };
 
-struct Location
+struct LocationXYZO {
-{
     float x, y, z, o;
 };
 
-static Location AllianceSpawn[] =
+static LocationXYZO AllianceSpawn[] =
 {
     { 1603.97f, 718.02f, 65.10f, 0  }, // guardian // sewers
     { 1604.78f, 657.22f, 40.80f, 0  }, // wave 1
@@ -788,7 +787,7 @@ static Location AllianceSpawn[] =
     { 1307.92f, 395.53f, -63.24f, 4.472f },
 };
 
-static Location AllianceWP[] =
+static LocationXYZO AllianceWP[] =
 {
     { 1737.06f, 734.176f, 48.8f, 0      }, // Jaina sewers UNUSED
     { 1682.92f, 730.89f, 76.84f, 0      }, // UNUSED
@@ -802,12 +801,12 @@ static Location AllianceWP[] =
     { 1300.75f, 347.39f, -65.02f, 0     }, // jaina throne room
 };
 
-static Location HordeSpawn[] =
+static LocationXYZO HordeSpawn[] =
 {
     { 1581.94f, 383.22f, -62.22f, 0 } // Khanok
 };
 
-static Location ThrallSpawn[] =
+static LocationXYZO ThrallSpawn[] =
 {
     // Vortex
     { 1880.0001f, 237.8242f, 59.472f, 3.060f  },

src/server/scripts/Kalimdor/zone_silithus.cpp

@@ -1078,7 +1078,7 @@ public:
 
         bool GossipSelect(Player* player, uint32 sender, uint32 action) override
         {
-            Seconds respawnTimer = 0s;
+            Seconds respawnTimer{};
             player->PlayerTalkClass->SendCloseGossip();
 
             Creature* lastSpawn = ObjectAccessor::GetCreature(*me, _creatureGuid);

src/server/scripts/Northrend/IcecrownCitadel/boss_professor_putricide.cpp

@@ -1008,9 +1008,12 @@ public:
                     break;
                 }
 
-            if (Aura* aura = target->GetAura(uint32(GetSpellInfo()->Effects[stage].CalcValue())))
+            if (target)
-                if (aura->GetOwner() == target) // avoid assert(false) at any cost
+            {
-                    aura->UpdateOwner(5000, target); // update whole aura so previous periodic ticks before refreshed by new one
+                if (Aura* aura = target->GetAura(uint32(GetSpellInfo()->Effects[stage].CalcValue())))
+                    if (aura->GetOwner() == target) // avoid assert(false) at any cost
+                        aura->UpdateOwner(5000, target); // update whole aura so previous periodic ticks before refreshed by new one
+            }
 
             GetCaster()->CastSpell(target, uint32(GetSpellInfo()->Effects[stage].CalcValue()), true, nullptr, nullptr, GetCaster()->GetGUID());
         }

src/server/scripts/Outland/zone_shadowmoon_valley.cpp

@@ -870,13 +870,13 @@ static Location SpawnLocation[] =
     {-4627.1240f, 1378.8752f, 139.9f, 2.544f} //Torloth The Magnificent
 };
 
-struct WaveData
+struct WaveDataCreature
 {
     uint8 SpawnCount, UsedSpawnPoint;
     uint32 CreatureId, SpawnTimer, YellTimer;
 };
 
-static WaveData WavesInfo[] =
+static WaveDataCreature WavesInfo[] =
 {
     {9, 0, 22075, 10000, 7000},   //Illidari Soldier
     {2, 9, 22074, 10000, 7000},   //Illidari Mind Breaker

src/server/shared/DataStores/DBCDatabaseLoader.cpp

@@ -122,7 +122,10 @@ char* DBCDatabaseLoader::Load(uint32& records, char**& indexTable)
 
     // insert new records to index table
     for (uint32 i = 0; i < newRecords; ++i)
+    {
+        // cppcheck-suppress autoVariables
         indexTable[newIndexes[i]] = &dataTable[i * _recordSize];
+    }
 
     records = indexTableSize;
 

src/tools/map_extractor/System.cpp

@@ -57,6 +57,7 @@
 #endif
 extern ArchiveSet gOpenArchives;
 
+// cppcheck-suppress ctuOneDefinitionRuleViolation
 typedef struct
 {
     char name[64];

src/tools/map_extractor/dbcfile.h

@@ -21,6 +21,7 @@
 #include <string>
 #include <utility>
 
+// cppcheck-suppress ctuOneDefinitionRuleViolation
 class DBCFile
 {
 public:
@@ -48,8 +49,8 @@ public:
         { }
     };
     // Iteration over database
-    class Iterator;
+    class Iterator; // cppcheck-suppress ctuOneDefinitionRuleViolation
-    class Record
+    class Record // cppcheck-suppress ctuOneDefinitionRuleViolation
     {
     public:
         [[nodiscard]] float getFloat(size_t field) const

src/tools/map_extractor/loadlib/loadlib.h

@@ -32,6 +32,7 @@ union u_map_fcc
 //
 // File version chunk
 //
+// cppcheck-suppress ctuOneDefinitionRuleViolation
 struct file_MVER
 {
     union

src/tools/map_extractor/mpq_libmpq04.h

@@ -28,6 +28,7 @@
 
 using namespace std;
 
+// cppcheck-suppress ctuOneDefinitionRuleViolation
 class MPQArchive
 {
 public:
@@ -69,6 +70,7 @@ public:
 };
 typedef std::deque<MPQArchive*> ArchiveSet;
 
+// cppcheck-suppress ctuOneDefinitionRuleViolation
 class MPQFile
 {
     //MPQHANDLE handle;

src/tools/mmaps_generator/MapBuilder.cpp

@@ -340,7 +340,7 @@ namespace MMAP
         {
             fclose(file);
             delete[] verts;
-            delete[] inds;
+            delete[] inds; // cppcheck-suppress uninitdata
             return;
         }