Skip to content

Commit 95fcdb4

Browse files
ottmlFlow86
ottml
authored and
Flow86
committed
Code review: Use consistent cast and parse argument as reference. Remove not used code where goal is nullptr because this can never happen
1 parent e6e734e commit 95fcdb4

4 files changed

Lines changed: 51 additions & 64 deletions

File tree

‎libs/common/include/commonDefines.h‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// Copyright (C) 2005 - 2021 Settlers Freaks (sf-team at siedler25.org)
1+
// Copyright (C) 2005 - 2026 Settlers Freaks (sf-team at siedler25.org)
22
//
33
// SPDX-License-Identifier: GPL-2.0-or-later
44

@@ -32,6 +32,13 @@ inline T checkedCast(T_Src* src)
3232
return static_cast<T>(src);
3333
}
3434

35+
template<typename T, typename T_Src>
36+
inline T& checkedCast(T_Src& src)
37+
{
38+
RTTR_Assert(dynamic_cast<T*>(&src));
39+
return static_cast<T&>(src);
40+
}
41+
3542
// Fwd decl
3643
namespace boost {
3744
namespace filesystem {

‎libs/s25main/buildings/nobBaseWarehouse.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -243,7 +243,7 @@ bool nobBaseWarehouse::OrderJob(const Job job, noRoadNode* const goal, const boo
243243
return false;
244244
}
245245

246-
std::unique_ptr<noFigure> fig = JobFactory::CreateJob(job, pos, player, goal);
246+
std::unique_ptr<noFigure> fig = JobFactory::CreateJob(job, pos, player, *goal);
247247
// Ziel Bescheid sagen, dass dortin ein neuer Arbeiter kommt (bei Flaggen als das anders machen)
248248
if(goal->GetType() != NodalObjectType::Flag)
249249
checkedCast<noBaseBuilding*>(goal)->GotWorker(job, *fig);

‎libs/s25main/factories/JobFactory.cpp‎

Lines changed: 40 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -49,90 +49,70 @@
4949
#include "nodeObjs/noFlag.h"
5050
#include <stdexcept>
5151

52-
namespace {
53-
nobUsual* staticCastAndAssertCorrectType(noRoadNode* const goal)
54-
{
55-
RTTR_Assert(dynamic_cast<nobUsual*>(goal));
56-
return static_cast<nobUsual*>(goal);
57-
}
58-
} // namespace
59-
6052
std::unique_ptr<noFigure> JobFactory::CreateJob(const Job job_id, const MapPoint pt, const unsigned char player,
61-
noRoadNode* const goal)
53+
noRoadNode& goal)
6254
{
6355
switch(job_id)
6456
{
6557
case Job::Builder:
66-
if(!goal)
67-
return std::make_unique<nofBuilder>(pt, player, nullptr);
68-
else if(goal->GetGOT() != GO_Type::Buildingsite)
69-
return std::make_unique<nofPassiveWorker>(Job::Builder, pt, player, goal);
58+
if(goal.GetGOT() != GO_Type::Buildingsite)
59+
return std::make_unique<nofPassiveWorker>(Job::Builder, pt, player, &goal);
7060
else
71-
return std::make_unique<nofBuilder>(pt, player, static_cast<noBuildingSite*>(goal));
72-
case Job::Planer:
73-
RTTR_Assert(dynamic_cast<noBuildingSite*>(goal));
74-
return std::make_unique<nofPlaner>(pt, player, static_cast<noBuildingSite*>(goal));
75-
case Job::Carpenter: return std::make_unique<nofCarpenter>(pt, player, staticCastAndAssertCorrectType(goal));
76-
case Job::Armorer: return std::make_unique<nofArmorer>(pt, player, staticCastAndAssertCorrectType(goal));
77-
case Job::Stonemason: return std::make_unique<nofStonemason>(pt, player, staticCastAndAssertCorrectType(goal));
78-
case Job::Brewer: return std::make_unique<nofBrewer>(pt, player, staticCastAndAssertCorrectType(goal));
79-
case Job::Minter: return std::make_unique<nofMinter>(pt, player, staticCastAndAssertCorrectType(goal));
80-
case Job::Butcher: return std::make_unique<nofButcher>(pt, player, staticCastAndAssertCorrectType(goal));
81-
case Job::IronFounder:
82-
return std::make_unique<nofIronfounder>(pt, player, staticCastAndAssertCorrectType(goal));
83-
case Job::Miller: return std::make_unique<nofMiller>(pt, player, staticCastAndAssertCorrectType(goal));
84-
case Job::Metalworker:
85-
return std::make_unique<nofMetalworker>(pt, player, staticCastAndAssertCorrectType(goal));
86-
case Job::Baker: return std::make_unique<nofBaker>(pt, player, staticCastAndAssertCorrectType(goal));
61+
return std::make_unique<nofBuilder>(pt, player, &checkedCast<noBuildingSite>(goal));
62+
case Job::Planer: return std::make_unique<nofPlaner>(pt, player, &checkedCast<noBuildingSite>(goal));
63+
case Job::Carpenter: return std::make_unique<nofCarpenter>(pt, player, &checkedCast<nobUsual>(goal));
64+
case Job::Armorer: return std::make_unique<nofArmorer>(pt, player, &checkedCast<nobUsual>(goal));
65+
case Job::Stonemason: return std::make_unique<nofStonemason>(pt, player, &checkedCast<nobUsual>(goal));
66+
case Job::Brewer: return std::make_unique<nofBrewer>(pt, player, &checkedCast<nobUsual>(goal));
67+
case Job::Minter: return std::make_unique<nofMinter>(pt, player, &checkedCast<nobUsual>(goal));
68+
case Job::Butcher: return std::make_unique<nofButcher>(pt, player, &checkedCast<nobUsual>(goal));
69+
case Job::IronFounder: return std::make_unique<nofIronfounder>(pt, player, &checkedCast<nobUsual>(goal));
70+
case Job::Miller: return std::make_unique<nofMiller>(pt, player, &checkedCast<nobUsual>(goal));
71+
case Job::Metalworker: return std::make_unique<nofMetalworker>(pt, player, &checkedCast<nobUsual>(goal));
72+
case Job::Baker: return std::make_unique<nofBaker>(pt, player, &checkedCast<nobUsual>(goal));
8773
case Job::Helper:
88-
if(goal && goal->GetGOT() == GO_Type::NobUsual)
74+
if(goal.GetGOT() == GO_Type::NobUsual)
8975
{
90-
auto* goalBld = staticCastAndAssertCorrectType(goal);
76+
auto* goalBld = &checkedCast<nobUsual>(goal);
9177
if(goalBld->GetBuildingType() == BuildingType::Well)
9278
return std::make_unique<nofWellguy>(pt, player, goalBld);
9379
else if(goalBld->GetBuildingType() == BuildingType::Catapult)
9480
return std::make_unique<nofCatapultMan>(pt, player, goalBld);
9581
}
96-
throw std::runtime_error("Invalid goal type: " + (goal ? helpers::toString(goal->GetGOT()) : "NULL")
97-
+ " for job " + helpers::toString(job_id));
98-
case Job::Geologist:
99-
RTTR_Assert(dynamic_cast<noFlag*>(goal));
100-
return std::make_unique<nofGeologist>(pt, player, static_cast<noFlag*>(goal));
82+
throw std::runtime_error("Invalid goal type: " + helpers::toString(goal.GetGOT()) + " for job "
83+
+ helpers::toString(job_id));
84+
case Job::Geologist: return std::make_unique<nofGeologist>(pt, player, &checkedCast<noFlag>(goal));
10185
case Job::Scout:
10286
// Different scout for lookout towers and free scouts
103-
if(goal->GetGOT() == GO_Type::NobUsual)
87+
if(goal.GetGOT() == GO_Type::NobUsual)
10488
{
105-
return std::make_unique<nofScout_LookoutTower>(pt, player, staticCastAndAssertCorrectType(goal));
106-
} else if(goal->GetGOT() == GO_Type::Flag)
107-
return std::make_unique<nofScout_Free>(pt, player, goal);
108-
throw std::runtime_error("Invalid goal type: " + helpers::toString(goal->GetGOT()) + " for job "
89+
return std::make_unique<nofScout_LookoutTower>(pt, player, &checkedCast<nobUsual>(goal));
90+
} else if(goal.GetGOT() == GO_Type::Flag)
91+
return std::make_unique<nofScout_Free>(pt, player, &goal);
92+
throw std::runtime_error("Invalid goal type: " + helpers::toString(goal.GetGOT()) + " for job "
10993
+ helpers::toString(job_id));
110-
case Job::Miner: return std::make_unique<nofMiner>(pt, player, staticCastAndAssertCorrectType(goal));
111-
case Job::Farmer: return std::make_unique<nofFarmer>(pt, player, staticCastAndAssertCorrectType(goal));
112-
case Job::Forester: return std::make_unique<nofForester>(pt, player, staticCastAndAssertCorrectType(goal));
113-
case Job::Woodcutter: return std::make_unique<nofWoodcutter>(pt, player, staticCastAndAssertCorrectType(goal));
114-
case Job::PigBreeder: return std::make_unique<nofPigbreeder>(pt, player, staticCastAndAssertCorrectType(goal));
115-
case Job::DonkeyBreeder:
116-
return std::make_unique<nofDonkeybreeder>(pt, player, staticCastAndAssertCorrectType(goal));
117-
case Job::Hunter: return std::make_unique<nofHunter>(pt, player, staticCastAndAssertCorrectType(goal));
118-
case Job::Fisher: return std::make_unique<nofFisher>(pt, player, staticCastAndAssertCorrectType(goal));
94+
case Job::Miner: return std::make_unique<nofMiner>(pt, player, &checkedCast<nobUsual>(goal));
95+
case Job::Farmer: return std::make_unique<nofFarmer>(pt, player, &checkedCast<nobUsual>(goal));
96+
case Job::Forester: return std::make_unique<nofForester>(pt, player, &checkedCast<nobUsual>(goal));
97+
case Job::Woodcutter: return std::make_unique<nofWoodcutter>(pt, player, &checkedCast<nobUsual>(goal));
98+
case Job::PigBreeder: return std::make_unique<nofPigbreeder>(pt, player, &checkedCast<nobUsual>(goal));
99+
case Job::DonkeyBreeder: return std::make_unique<nofDonkeybreeder>(pt, player, &checkedCast<nobUsual>(goal));
100+
case Job::Hunter: return std::make_unique<nofHunter>(pt, player, &checkedCast<nobUsual>(goal));
101+
case Job::Fisher: return std::make_unique<nofFisher>(pt, player, &checkedCast<nobUsual>(goal));
119102
case Job::Private:
120103
case Job::PrivateFirstClass:
121104
case Job::Sergeant:
122105
case Job::Officer:
123106
case Job::General:
124107
// TODO: Is this ever called? If yes, then why is the home here set to nullptr?
125-
RTTR_Assert(dynamic_cast<nobBaseMilitary*>(goal));
126-
return std::make_unique<nofPassiveSoldier>(pt, player, static_cast<nobBaseMilitary*>(goal), nullptr,
108+
return std::make_unique<nofPassiveSoldier>(pt, player, &checkedCast<nobBaseMilitary>(goal), nullptr,
127109
getSoldierRank(job_id));
128-
case Job::PackDonkey: return std::make_unique<nofCarrier>(CarrierType::Donkey, pt, player, nullptr, goal);
129-
case Job::Shipwright: return std::make_unique<nofShipWright>(pt, player, staticCastAndAssertCorrectType(goal));
130-
case Job::CharBurner: return std::make_unique<nofCharburner>(pt, player, staticCastAndAssertCorrectType(goal));
131-
case Job::Winegrower: return std::make_unique<nofWinegrower>(pt, player, staticCastAndAssertCorrectType(goal));
132-
case Job::Vintner: return std::make_unique<nofVintner>(pt, player, staticCastAndAssertCorrectType(goal));
133-
case Job::TempleServant:
134-
RTTR_Assert(dynamic_cast<nobUsual*>(goal));
135-
return std::make_unique<nofTempleServant>(pt, player, staticCastAndAssertCorrectType(goal));
110+
case Job::PackDonkey: return std::make_unique<nofCarrier>(CarrierType::Donkey, pt, player, nullptr, &goal);
111+
case Job::Shipwright: return std::make_unique<nofShipWright>(pt, player, &checkedCast<nobUsual>(goal));
112+
case Job::CharBurner: return std::make_unique<nofCharburner>(pt, player, &checkedCast<nobUsual>(goal));
113+
case Job::Winegrower: return std::make_unique<nofWinegrower>(pt, player, &checkedCast<nobUsual>(goal));
114+
case Job::Vintner: return std::make_unique<nofVintner>(pt, player, &checkedCast<nobUsual>(goal));
115+
case Job::TempleServant: return std::make_unique<nofTempleServant>(pt, player, &checkedCast<nobUsual>(goal));
136116
case Job::BoatCarrier:
137117
throw std::logic_error("Cannot create a boat carrier job (try creating Job::Helper).");
138118
break;

‎libs/s25main/factories/JobFactory.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// Copyright (C) 2005 - 2021 Settlers Freaks (sf-team at siedler25.org)
1+
// Copyright (C) 2005 - 2026 Settlers Freaks (sf-team at siedler25.org)
22
//
33
// SPDX-License-Identifier: GPL-2.0-or-later
44

@@ -18,5 +18,5 @@ class JobFactory
1818
JobFactory() = delete;
1919

2020
// Erstellt Job anhand der job-id
21-
static std::unique_ptr<noFigure> CreateJob(Job job_id, MapPoint pt, unsigned char player, noRoadNode* goal);
21+
static std::unique_ptr<noFigure> CreateJob(Job job_id, MapPoint pt, unsigned char player, noRoadNode& goal);
2222
};

0 commit comments

Comments
 (0)