CppCheck 2.15 performance cleanup (pioneerspacesim#5931)

* In summary, stop passing raw std::string! Upgrade CppCheck to 2.15 and ran it over the codebase. Tackled the most obvious performance warnings plus a few others. LOTS of std::string being passed instead of const references, along with vectors, maps, and other types which involve lots of copying
robothauler · Oct 8, 2024 · 770ed87 · 770ed87
1 parent 6bac7c9
commit 770ed87
Show file tree

Hide file tree

Showing 53 changed files with 181 additions and 180 deletions.
diff --git a/scripts/Pioneer.cppcheck b/scripts/Pioneer.cppcheck
@@ -1,5 +1,12 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <project version="1">
+    <platform>win64</platform>
+    <analyze-all-vs-configs>false</analyze-all-vs-configs>
+    <check-headers>true</check-headers>
+    <check-unused-templates>true</check-unused-templates>
+    <inline-suppression>true</inline-suppression>
+    <max-ctu-depth>2</max-ctu-depth>
+    <max-template-recursion>100</max-template-recursion>
     <paths>
         <dir name="../src"/>
     </paths>

diff --git a/src/Background.cpp b/src/Background.cpp
@@ -262,7 +262,7 @@ namespace Background {
 
 	class SampleStarsTask : public Task {
 	public:
-		SampleStarsTask(RefCountedPtr<Galaxy> galaxy, StarQueryInfo info, int32_t starsLimit, StarInfo &stars, double &medianBrightness, TaskRange range) :
+		SampleStarsTask(RefCountedPtr<Galaxy> galaxy, const StarQueryInfo &info, int32_t starsLimit, StarInfo &stars, double &medianBrightness, TaskRange range) :
 			Task(range),
 			galaxy(galaxy),
 			info(info),
@@ -374,7 +374,7 @@ namespace Background {
 
 	class SortStarsTask : public Task {
 	public:
-		SortStarsTask(StarQueryInfo info, StarInfo &stars, double medianBrightness) :
+		SortStarsTask(const StarQueryInfo &info, StarInfo &stars, double medianBrightness) :
 			info(info),
 			stars(stars),
 			medianBrightness(medianBrightness)

diff --git a/src/BaseSphere.cpp b/src/BaseSphere.cpp
@@ -43,15 +43,15 @@ BaseSphere::~BaseSphere() {}
 void BaseSphere::Init()
 {
 	PROFILE_SCOPED()
-	GeoSphere::Init();
-	GasGiant::Init();
+	GeoSphere::InitGeoSphere();
+	GasGiant::InitGasGiant();
 }
 
 //static
 void BaseSphere::Uninit()
 {
-	GeoSphere::Uninit();
-	GasGiant::Uninit();
+	GeoSphere::UninitGeoSphere();
+	GasGiant::UninitGasGiant();
 }
 
 //static
@@ -64,7 +64,7 @@ void BaseSphere::UpdateAllBaseSphereDerivatives()
 //static
 void BaseSphere::OnChangeDetailLevel()
 {
-	GeoSphere::OnChangeDetailLevel();
+	GeoSphere::OnChangeGeoSphereDetailLevel();
 }
 
 void BaseSphere::DrawAtmosphereSurface(Graphics::Renderer *renderer,

diff --git a/src/FileSystem.cpp b/src/FileSystem.cpp
@@ -146,7 +146,7 @@ namespace FileSystem {
 		return path;
 	}
 
-	bool CopyDir(FileSource &sourceFS, std::string sourceDir, FileSourceFS &targetFS, std::string targetDir, FileSystem::CopyMode copymode)
+	bool CopyDir(FileSource &sourceFS, const std::string &sourceDir, FileSourceFS &targetFS, const std::string &targetDir, FileSystem::CopyMode copymode)
 	{
 		// NOTE: copymode var is not used, because only mode ONLY_MISSING_IN_TARGET is implemented
 		if (!sourceFS.Lookup(sourceDir).IsDir() || !targetFS.Lookup(targetDir).IsDir())

diff --git a/src/FileSystem.h b/src/FileSystem.h
@@ -73,7 +73,7 @@ namespace FileSystem {
 
 	/// Copy the contents of a directory from sourceFS into a directory in targetFS, according to copymode.
 	/// Returns false if sourceDir or targetDir are invalid
-	bool CopyDir(FileSource &sourceFS, std::string sourceDir, FileSourceFS &targetFS, std::string targetDir, CopyMode copymode = CopyMode::OVERWRITE);
+	bool CopyDir(FileSource &sourceFS, const std::string &sourceDir, FileSourceFS &targetFS, const std::string &targetDir, CopyMode copymode = CopyMode::OVERWRITE);
 
 	class FileInfo {
 		friend class FileSource;

diff --git a/src/GasGiant.cpp b/src/GasGiant.cpp
@@ -135,7 +135,7 @@ class GasPatchContext : public RefCounted {
 
 		// add all of the middle indices
 		for (int i = 0; i < IDX_VBO_COUNT_ALL_IDX(); ++i) {
-			pl.push_back(indices[i]);
+			pl.emplace_back(indices[i]);
 		}
 
 		return tri_count;
@@ -303,7 +303,7 @@ GasGiant::GasGiant(const SystemBody *body) :
 	m_hasGpuJobRequest(false),
 	m_timeDelay(s_initialCPUDelayTime)
 {
-	s_allGasGiants.push_back(this);
+	s_allGasGiants.emplace_back(this);
 
 	for (int i = 0; i < NUM_PATCHES; i++) {
 		m_hasJobRequest[i] = false;
@@ -741,7 +741,7 @@ void GasGiant::BuildFirstPatches()
 	GenerateTexture();
 }
 
-void GasGiant::Init()
+void GasGiant::InitGasGiant()
 {
 	IniConfig cfg;
 	cfg.Read(FileSystem::gameDataFiles, "configs/GasGiants.ini");
@@ -764,7 +764,7 @@ void GasGiant::Init()
 	CreateRenderTarget(s_texture_size_gpu[Pi::detail.planets], s_texture_size_gpu[Pi::detail.planets]);
 }
 
-void GasGiant::Uninit()
+void GasGiant::UninitGasGiant()
 {
 	s_patchContext.Reset();
 }

diff --git a/src/GasGiant.h b/src/GasGiant.h
@@ -47,16 +47,14 @@ class GasGiant : public BaseSphere {
 
 	static bool OnAddTextureFaceResult(const SystemPath &path, GasGiantJobs::STextureFaceResult *res);
 	static bool OnAddGPUGenResult(const SystemPath &path, GasGiantJobs::SGPUGenResult *res);
-	static void Init();
-	static void Uninit();
+	static void InitGasGiant();
+	static void UninitGasGiant();
 	static void UpdateAllGasGiants();
 	static void OnChangeDetailLevel();
 
 	static void CreateRenderTarget(const Uint16 width, const Uint16 height);
 	static void SetRenderTargetCubemap(const Uint32, Graphics::Texture *, const bool unBind = true);
 	static Graphics::RenderTarget *GetRenderTarget();
-	static void BeginRenderTarget();
-	static void EndRenderTarget();
 
 private:
 	void BuildFirstPatches();

diff --git a/src/GeoSphere.cpp b/src/GeoSphere.cpp
@@ -40,12 +40,12 @@ static const int detail_edgeLen[5] = {
 static const double gs_targetPatchTriLength(100.0);
 static std::vector<GeoSphere *> s_allGeospheres;
 
-void GeoSphere::Init()
+void GeoSphere::InitGeoSphere()
 {
 	s_patchContext.Reset(new GeoPatchContext(detail_edgeLen[Pi::detail.planets > 4 ? 4 : Pi::detail.planets]));
 }
 
-void GeoSphere::Uninit()
+void GeoSphere::UninitGeoSphere()
 {
 	assert(s_patchContext.Unique());
 	s_patchContext.Reset();
@@ -71,7 +71,7 @@ void GeoSphere::UpdateAllGeoSpheres()
 }
 
 // static
-void GeoSphere::OnChangeDetailLevel()
+void GeoSphere::OnChangeGeoSphereDetailLevel()
 {
 	s_patchContext.Reset(new GeoPatchContext(detail_edgeLen[Pi::detail.planets > 4 ? 4 : Pi::detail.planets]));
 
@@ -187,7 +187,7 @@ GeoSphere::GeoSphere(const SystemBody *body) :
 {
 	print_info(body, m_terrain.Get());
 
-	s_allGeospheres.push_back(this);
+	s_allGeospheres.emplace_back(this);
 
 	CalculateMaxPatchDepth();
 
@@ -207,7 +207,7 @@ bool GeoSphere::AddQuadSplitResult(SQuadSplitResult *res)
 	assert(res);
 	assert(mQuadSplitResults.size() < MAX_SPLIT_OPERATIONS);
 	if (mQuadSplitResults.size() < MAX_SPLIT_OPERATIONS) {
-		mQuadSplitResults.push_back(res);
+		mQuadSplitResults.emplace_back(res);
 		result = true;
 	}
 	return result;
@@ -219,7 +219,7 @@ bool GeoSphere::AddSingleSplitResult(SSingleSplitResult *res)
 	assert(res);
 	assert(mSingleSplitResults.size() < MAX_SPLIT_OPERATIONS);
 	if (mSingleSplitResults.size() < MAX_SPLIT_OPERATIONS) {
-		mSingleSplitResults.push_back(res);
+		mSingleSplitResults.emplace_back(res);
 		result = true;
 	}
 	return result;
@@ -360,7 +360,7 @@ void GeoSphere::Update()
 
 void GeoSphere::AddQuadSplitRequest(double dist, SQuadSplitRequest *pReq, GeoPatch *pPatch)
 {
-	mQuadSplitRequests.push_back(TDistanceRequest(dist, pReq, pPatch));
+	mQuadSplitRequests.emplace_back(dist, pReq, pPatch);
 }
 
 void GeoSphere::ProcessQuadSplitRequests()

diff --git a/src/GeoSphere.h b/src/GeoSphere.h
@@ -50,10 +50,10 @@ class GeoSphere : public BaseSphere {
 		return h;
 	}
 
-	static void Init();
-	static void Uninit();
+	static void InitGeoSphere();
+	static void UninitGeoSphere();
 	static void UpdateAllGeoSpheres();
-	static void OnChangeDetailLevel();
+	static void OnChangeGeoSphereDetailLevel();
 	static bool OnAddQuadSplitResult(const SystemPath &path, SQuadSplitResult *res);
 	static bool OnAddSingleSplitResult(const SystemPath &path, SSingleSplitResult *res);
 	// in sbody radii

diff --git a/src/HudTrail.cpp b/src/HudTrail.cpp
@@ -16,11 +16,10 @@ const Uint16 MAX_POINTS = 100;
 
 HudTrail::HudTrail(Body *b, const Color &c) :
 	m_body(b),
+	m_currentFrame(b->GetFrame()),
 	m_updateTime(0.f),
 	m_color(c)
 {
-	m_currentFrame = b->GetFrame();
-
 	Graphics::MaterialDescriptor desc;
 
 	Graphics::RenderStateDesc rsd;
@@ -45,7 +44,7 @@ void HudTrail::Update(float time)
 		}
 
 		if (bodyFrameId == m_currentFrame)
-			m_trailPoints.push_back(m_body->GetInterpPosition());
+			m_trailPoints.emplace_back(m_body->GetInterpPosition());
 	}
 
 	while (m_trailPoints.size() > MAX_POINTS)
@@ -71,15 +70,15 @@ void HudTrail::Render(Graphics::Renderer *r)
 		tvts.reserve(MAX_POINTS);
 		colors.reserve(MAX_POINTS);
 
-		tvts.push_back(vector3f(0.f));
-		colors.push_back(Color::BLANK);
+		tvts.emplace_back(vector3f(0.f));
+		colors.emplace_back(Color::BLANK);
 		float alpha = 1.f;
 		const float decrement = 1.f / m_trailPoints.size();
 		const Color tcolor = m_color;
 		for (size_t i = m_trailPoints.size() - 1; i > 0; i--) {
-			tvts.push_back(-vector3f(curpos - m_trailPoints[i]));
+			tvts.emplace_back(-vector3f(curpos - m_trailPoints[i]));
 			alpha -= decrement;
-			colors.push_back(tcolor);
+			colors.emplace_back(tcolor);
 			colors.back().a = Uint8(alpha * 255);
 		}
 

diff --git a/src/Input.cpp b/src/Input.cpp
@@ -285,7 +285,7 @@ void Manager::InitGame()
 
 */
 
-InputBindings::Action *InputFrame::AddAction(std::string id)
+InputBindings::Action *InputFrame::AddAction(const std::string &id)
 {
 	auto *action = manager->GetActionBinding(id);
 	if (!action)
@@ -295,7 +295,7 @@ InputBindings::Action *InputFrame::AddAction(std::string id)
 	return action;
 }
 
-InputBindings::Axis *InputFrame::AddAxis(std::string id)
+InputBindings::Axis *InputFrame::AddAxis(const std::string &id)
 {
 	auto *axis = manager->GetAxisBinding(id);
 	if (!axis)
@@ -368,7 +368,7 @@ void Manager::RemoveInputFrame(InputFrame *frame)
 	}
 }
 
-InputBindings::Action *Manager::AddActionBinding(std::string id, BindingGroup *group, InputBindings::Action &&binding)
+InputBindings::Action *Manager::AddActionBinding(const std::string &id, BindingGroup *group, InputBindings::Action &&binding)
 {
 	// throw an error if we attempt to bind an action onto an already-bound axis in the same group.
 	if (group->bindings.count(id) && group->bindings[id] != BindingGroup::ENTRY_ACTION)
@@ -386,7 +386,7 @@ InputBindings::Action *Manager::AddActionBinding(std::string id, BindingGroup *g
 	return &(actionBindings[id] = binding);
 }
 
-InputBindings::Axis *Manager::AddAxisBinding(std::string id, BindingGroup *group, InputBindings::Axis &&binding)
+InputBindings::Axis *Manager::AddAxisBinding(const std::string &id, BindingGroup *group, InputBindings::Axis &&binding)
 {
 	// throw an error if we attempt to bind an axis onto an already-bound action in the same group.
 	if (group->bindings.count(id) && group->bindings[id] != BindingGroup::ENTRY_AXIS)
@@ -404,12 +404,12 @@ InputBindings::Axis *Manager::AddAxisBinding(std::string id, BindingGroup *group
 	return &(axisBindings[id] = binding);
 }
 
-InputBindings::Action *Manager::GetActionBinding(std::string id)
+InputBindings::Action *Manager::GetActionBinding(const std::string &id)
 {
 	return actionBindings.count(id) ? &actionBindings[id] : &Input::nullAction;
 }
 
-InputBindings::Axis *Manager::GetAxisBinding(std::string id)
+InputBindings::Axis *Manager::GetAxisBinding(const std::string &id)
 {
 	return axisBindings.count(id) ? &axisBindings[id] : &Input::nullAxis;
 }

diff --git a/src/Input.h b/src/Input.h
@@ -40,7 +40,7 @@ namespace Input {
 	};
 
 	struct BindingPage {
-		BindingGroup *GetBindingGroup(std::string id) { return &groups[id]; }
+		BindingGroup *GetBindingGroup(const std::string &id) { return &groups[id]; }
 
 		std::map<std::string, BindingGroup> groups;
 	};
@@ -71,8 +71,8 @@ namespace Input {
 		// Called when the frame is removed from the stack.
 		sigc::signal<void, InputFrame *> onFrameRemoved;
 
-		Action *AddAction(std::string id);
-		Axis *AddAxis(std::string id);
+		Action *AddAction(const std::string &id);
+		Axis *AddAxis(const std::string &id);
 	};
 
 	struct JoystickInfo {
@@ -138,8 +138,8 @@ class Input::Manager {
 	// When enable is false, this prevents the input system from writing to the config file.
 	void EnableConfigSaving(bool enable) { m_enableConfigSaving = enable; }
 
-	BindingPage *GetBindingPage(std::string id) { return &bindingPages[id]; }
-	std::map<std::string, BindingPage> GetBindingPages() { return bindingPages; }
+	BindingPage *GetBindingPage(const std::string &id) { return &bindingPages[id]; }
+	const std::map<std::string, BindingPage>& GetBindingPages() const { return bindingPages; }
 
 	// Pushes an InputFrame onto the input stack.
 	bool AddInputFrame(InputFrame *frame);
@@ -158,13 +158,13 @@ class Input::Manager {
 
 	// Creates a new action binding, copying the provided binding.
 	// The returned binding pointer points to the actual binding.
-	InputBindings::Action *AddActionBinding(std::string id, BindingGroup *group, InputBindings::Action &&binding);
-	InputBindings::Action *GetActionBinding(std::string id);
+	InputBindings::Action *AddActionBinding(const std::string &id, BindingGroup *group, InputBindings::Action &&binding);
+	InputBindings::Action *GetActionBinding(const std::string &id);
 
 	// Creates a new axis binding, copying the provided binding.
 	// The returned binding pointer points to the actual binding.
-	InputBindings::Axis *AddAxisBinding(std::string id, BindingGroup *group, InputBindings::Axis &&binding);
-	InputBindings::Axis *GetAxisBinding(std::string id);
+	InputBindings::Axis *AddAxisBinding(const std::string &id, BindingGroup *group, InputBindings::Axis &&binding);
+	InputBindings::Axis *GetAxisBinding(const std::string &id);
 
 	// Call EnableBindings() to temporarily disable handling input bindings while
 	// you're recording a new input binding or are in a modal window.

diff --git a/src/Intro.cpp b/src/Intro.cpp
@@ -41,7 +41,7 @@ Intro::Intro(Graphics::Renderer *r, int width, int height) :
 	m_skin.SetDecal("pioneer");
 	m_skin.SetLabel(Lang::PIONEER);
 
-	for (auto i : ShipType::player_ships) {
+	for (const auto &i : ShipType::player_ships) {
 		SceneGraph::Model *model = Pi::FindModel(ShipType::types[i].modelName)->MakeInstance();
 		model->SetThrust(vector3f(0.f, 0.f, -0.6f), vector3f(0.f));
 		if (ShipType::types[i].isGlobalColorDefined) model->SetThrusterColor(ShipType::types[i].globalThrusterColor);
@@ -58,10 +58,6 @@ Intro::Intro(Graphics::Renderer *r, int width, int height) :
 			}
 			model->SetThrusterColor(dir, ShipType::types[i].directionThrusterColor[j]);
 		}
-		const Uint32 numMats = model->GetNumMaterials();
-		for (Uint32 m = 0; m < numMats; m++) {
-			RefCountedPtr<Graphics::Material> mat = model->GetMaterialByIndex(m);
-		}
 		m_models.push_back(model);
 	}
 

diff --git a/src/ObjectViewerView.cpp b/src/ObjectViewerView.cpp
@@ -30,11 +30,10 @@ ObjectViewerView::ObjectViewerView() :
 	PiGuiView("ObjectViewerView"),
 	m_targetBody(nullptr),
 	m_systemBody(nullptr),
-	m_state{}
+	m_state{},
+	viewingDist(1000.0f),
+	m_camRot(matrix4x4d::Identity())
 {
-	viewingDist = 1000.0f;
-	m_camRot = matrix4x4d::Identity();
-
 	float znear;
 	float zfar;
 	Pi::renderer->GetNearFarRange(znear, zfar);

diff --git a/src/Pi.cpp b/src/Pi.cpp
@@ -159,7 +159,7 @@ class StartupScreen : public Application::Lifecycle {
 	bool m_hasQueuedJobs = 0;
 
 	template <typename T>
-	void AddStep(std::string name, T fn)
+	void AddStep(const std::string &name, T fn)
 	{
 		m_loaders.push_back(LoadStep{ fn, name });
 	}