From f1ded70366cb8a3a724ebd61bb3092fabb9a986f Mon Sep 17 00:00:00 2001 From: elsid Date: Wed, 6 Jul 2022 12:45:03 +0200 Subject: [PATCH 1/7] Remove redundant condition apps/openmw/mwmechanics/character.cpp:500:14: warning: redundant condition 'isRealWeapon' [bugprone-redundant-branch-condition] else if (isRealWeapon) ^~~~~~~~~~~~~~~~~ --- apps/openmw/mwmechanics/character.cpp | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/apps/openmw/mwmechanics/character.cpp b/apps/openmw/mwmechanics/character.cpp index f4df570a06..be46f3c06d 100644 --- a/apps/openmw/mwmechanics/character.cpp +++ b/apps/openmw/mwmechanics/character.cpp @@ -243,6 +243,13 @@ float getFallDamage(const MWWorld::Ptr& ptr, float fallHeight) return 0.f; } +bool isRealWeapon(int weaponType) +{ + return weaponType != ESM::Weapon::HandToHand + && weaponType != ESM::Weapon::Spell + && weaponType != ESM::Weapon::None; +} + } namespace MWMechanics @@ -486,8 +493,7 @@ void CharacterController::onClose() const std::string CharacterController::getWeaponAnimation(int weaponType) const { std::string weaponGroup = getWeaponType(weaponType)->mLongGroup; - bool isRealWeapon = weaponType != ESM::Weapon::HandToHand && weaponType != ESM::Weapon::Spell && weaponType != ESM::Weapon::None; - if (isRealWeapon && !mAnimation->hasAnimation(weaponGroup)) + if (isRealWeapon(weaponType) && !mAnimation->hasAnimation(weaponGroup)) { static const std::string oneHandFallback = getWeaponType(ESM::Weapon::LongBladeOneHand)->mLongGroup; static const std::string twoHandFallback = getWeaponType(ESM::Weapon::LongBladeTwoHand)->mLongGroup; @@ -497,7 +503,7 @@ std::string CharacterController::getWeaponAnimation(int weaponType) const // For real two-handed melee weapons use 2h swords animations as fallback, otherwise use the 1h ones if (weapInfo->mFlags & ESM::WeaponType::TwoHanded && weapInfo->mWeaponClass == ESM::WeaponType::Melee) weaponGroup = twoHandFallback; - else if (isRealWeapon) + else weaponGroup = oneHandFallback; } else if (weaponType == ESM::Weapon::HandToHand && !mPtr.getClass().isBipedal(mPtr)) @@ -515,8 +521,7 @@ std::string CharacterController::getWeaponShortGroup(int weaponType) const std::string CharacterController::fallbackShortWeaponGroup(const std::string& baseGroupName, MWRender::Animation::BlendMask* blendMask) const { - bool isRealWeapon = mWeaponType != ESM::Weapon::HandToHand && mWeaponType != ESM::Weapon::Spell && mWeaponType != ESM::Weapon::None; - if (!isRealWeapon) + if (!isRealWeapon(mWeaponType)) { if (blendMask != nullptr) *blendMask = MWRender::Animation::BlendMask_LowerBody; From 5b9ca3b979768e3cf93e1c4a2f830fcc9354f13f Mon Sep 17 00:00:00 2001 From: elsid Date: Wed, 6 Jul 2022 12:49:02 +0200 Subject: [PATCH 2/7] Avoid possible division by zero components/detournavigator/navmeshdb.cpp:183:43: warning: Division by zero [clang-analyzer-core.DivideZero] setMaxPageCount(*mDb, maxFileSize / dbPageSize + static_cast((maxFileSize % dbPageSize) != 0)); ~~~~~~~~~~~~^~~~~~~~~~~~ components/detournavigator/navmeshdb.cpp:182:33: note: Calling 'getPageSize' const auto dbPageSize = getPageSize(*mDb); ^~~~~~~~~~~~~~~~~ components/detournavigator/navmeshdb.cpp:144:13: note: 'value' initialized to 0 std::uint64_t value = 0; ^~~~~~~~~~~~~~~~~~~ components/detournavigator/navmeshdb.cpp:145:13: note: Calling 'request' request(db, statement, &value, 1); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ components/sqlite3/request.hpp:254:64: note: Left side of '&&' is false for (std::size_t i = 0; executeStep(db, statement) && i < max; ++i) ^ components/detournavigator/navmeshdb.cpp:145:13: note: Returning from 'request' request(db, statement, &value, 1); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ components/detournavigator/navmeshdb.cpp:146:13: note: Returning zero (loaded from 'value') return value; ^~~~~~~~~~~~ components/detournavigator/navmeshdb.cpp:182:33: note: Returning from 'getPageSize' const auto dbPageSize = getPageSize(*mDb); ^~~~~~~~~~~~~~~~~ components/detournavigator/navmeshdb.cpp:182:9: note: 'dbPageSize' initialized to 0 const auto dbPageSize = getPageSize(*mDb); ^~~~~~~~~~~~~~~~~~~~~ components/detournavigator/navmeshdb.cpp:183:43: note: Division by zero setMaxPageCount(*mDb, maxFileSize / dbPageSize + static_cast((maxFileSize % dbPageSize) != 0)); ~~~~~~~~~~~~^~~~~~~~~~~~ --- components/detournavigator/navmeshdb.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/components/detournavigator/navmeshdb.cpp b/components/detournavigator/navmeshdb.cpp index 4d9ab3dafa..93439387d7 100644 --- a/components/detournavigator/navmeshdb.cpp +++ b/components/detournavigator/navmeshdb.cpp @@ -179,7 +179,9 @@ namespace DetourNavigator , mInsertShape(*mDb, DbQueries::InsertShape {}) , mVacuum(*mDb, DbQueries::Vacuum {}) { - const auto dbPageSize = getPageSize(*mDb); + const std::uint64_t dbPageSize = getPageSize(*mDb); + if (dbPageSize == 0) + throw std::runtime_error("NavMeshDb page size is zero"); setMaxPageCount(*mDb, maxFileSize / dbPageSize + static_cast((maxFileSize % dbPageSize) != 0)); } From 7501597813b45f7998a31ed7af35754b91dd0577 Mon Sep 17 00:00:00 2001 From: elsid Date: Wed, 6 Jul 2022 12:55:36 +0200 Subject: [PATCH 3/7] Do not use float as loop variable apps/opencs/view/render/instanceselectionmode.cpp:294:9: warning: Variable 'i' with floating point type 'float' should not be used as a loop counter [clang-analyzer-security.FloatLoopCounter] for (float i = 0.0; i <= resolution; i += 2) ^ ~ ~ --- apps/opencs/view/render/instanceselectionmode.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/opencs/view/render/instanceselectionmode.cpp b/apps/opencs/view/render/instanceselectionmode.cpp index 9b5fb759cc..545d35d93b 100644 --- a/apps/opencs/view/render/instanceselectionmode.cpp +++ b/apps/opencs/view/render/instanceselectionmode.cpp @@ -284,14 +284,14 @@ namespace CSVRender osg::ref_ptr geometry (new osg::Geometry); osg::Vec3Array *vertices = new osg::Vec3Array; - int resolution = 32; + constexpr int resolution = 32; float radiusPerResolution = radius / resolution; float reciprocalResolution = 1.0f / resolution; float doubleReciprocalRes = reciprocalResolution * 2; osg::Vec4Array *colours = new osg::Vec4Array; - for (float i = 0.0; i <= resolution; i += 2) + for (int i = 0; i <= resolution; i += 2) { float iShifted = (static_cast(i) - resolution / 2.0f); // i - 16 = -16 ... 16 float xPercentile = iShifted * doubleReciprocalRes; From 72bda2bd101023a61f00838f988279661a868c3f Mon Sep 17 00:00:00 2001 From: elsid Date: Wed, 6 Jul 2022 12:58:12 +0200 Subject: [PATCH 4/7] Avoid redundant initialization components/nifbullet/bulletnifloader.cpp:79:24: warning: Value stored to 'a' during its initialization is never read [clang-analyzer-deadcode.DeadStores] unsigned short a = strip[0], b = strip[0], c = strip[1]; ^ ~~~~~~~~ --- components/nifbullet/bulletnifloader.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/components/nifbullet/bulletnifloader.cpp b/components/nifbullet/bulletnifloader.cpp index 424a45d65a..bedd2ce939 100644 --- a/components/nifbullet/bulletnifloader.cpp +++ b/components/nifbullet/bulletnifloader.cpp @@ -76,7 +76,9 @@ void fillTriangleMesh(btTriangleMesh& mesh, const Nif::NiTriStripsData& data, co if (strip.size() < 3) continue; - unsigned short a = strip[0], b = strip[0], c = strip[1]; + unsigned short a; + unsigned short b = strip[0]; + unsigned short c = strip[1]; for (size_t i = 2; i < strip.size(); i++) { a = b; From 4ecee2e16784b9b3b665fe754cf23bc23adfdfba Mon Sep 17 00:00:00 2001 From: elsid Date: Wed, 6 Jul 2022 13:01:03 +0200 Subject: [PATCH 5/7] Avoid using reserved identifier in the global namespace apps/launcher/datafilespage.cpp:762:12: warning: declaration uses identifier '_reloadCellsMutex', which is reserved in the global namespace [bugprone-reserved-identifier] std::mutex _reloadCellsMutex; ^~~~~~~~~~~~~~~~~ reloadCellsMutex apps/openmw/mwgui/journalwindow.cpp:86:103: warning: declaration uses identifier '_sender', which is reserved in the global namespace [bugprone-reserved-identifier] void adviseButtonClick (char const * name, void (JournalWindowImpl::*Handler) (MyGUI::Widget* _sender)) ^~~~~~~ sender apps/openmw/mwgui/journalwindow.cpp:92:100: warning: declaration uses identifier '_sender', which is reserved in the global namespace [bugprone-reserved-identifier] void adviseKeyPress (char const * name, void (JournalWindowImpl::*Handler) (MyGUI::Widget* _sender, MyGUI::KeyCode key, MyGUI::Char character)) ^~~~~~~ sender --- apps/launcher/datafilespage.cpp | 4 ++-- apps/openmw/mwgui/journalwindow.cpp | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/apps/launcher/datafilespage.cpp b/apps/launcher/datafilespage.cpp index cb94eddd78..e16202a047 100644 --- a/apps/launcher/datafilespage.cpp +++ b/apps/launcher/datafilespage.cpp @@ -759,13 +759,13 @@ void Launcher::DataFilesPage::slotAddonDataChanged() } // Mutex lock to run reloadCells synchronously. -std::mutex _reloadCellsMutex; +static std::mutex reloadCellsMutex; void Launcher::DataFilesPage::reloadCells(QStringList selectedFiles) { // Use a mutex lock so that we can prevent two threads from executing the rest of this code at the same time // Based on https://stackoverflow.com/a/5429695/531762 - std::unique_lock lock(_reloadCellsMutex); + std::unique_lock lock(reloadCellsMutex); // The following code will run only if there is not another thread currently running it CellNameLoader cellNameLoader; diff --git a/apps/openmw/mwgui/journalwindow.cpp b/apps/openmw/mwgui/journalwindow.cpp index 96c42549a5..d215587d49 100644 --- a/apps/openmw/mwgui/journalwindow.cpp +++ b/apps/openmw/mwgui/journalwindow.cpp @@ -83,16 +83,16 @@ namespace setVisible (visible); } - void adviseButtonClick (char const * name, void (JournalWindowImpl::*Handler) (MyGUI::Widget* _sender)) + void adviseButtonClick (char const * name, void (JournalWindowImpl::*handler)(MyGUI::Widget*)) { getWidget (name) -> - eventMouseButtonClick += newDelegate(this, Handler); + eventMouseButtonClick += newDelegate(this, handler); } - void adviseKeyPress (char const * name, void (JournalWindowImpl::*Handler) (MyGUI::Widget* _sender, MyGUI::KeyCode key, MyGUI::Char character)) + void adviseKeyPress (char const * name, void (JournalWindowImpl::*handler)(MyGUI::Widget*, MyGUI::KeyCode, MyGUI::Char)) { getWidget (name) -> - eventKeyButtonPressed += newDelegate(this, Handler); + eventKeyButtonPressed += newDelegate(this, handler); } MWGui::BookPage* getPage (char const * name) From bd7f56ddb45b8e1b5442e8054f1caadd0b26ed2c Mon Sep 17 00:00:00 2001 From: elsid Date: Wed, 6 Jul 2022 13:09:19 +0200 Subject: [PATCH 6/7] Don't rely on virtual dispatch in constructor apps/openmw/mwrender/animation.cpp:1841:60: warning: Call to virtual method 'ObjectAnimation::canBeHarvested' during construction bypasses virtual dispatch [clang-analyzer-optin.cplusplus.VirtualCall] if (ptr.getRefData().getCustomData() != nullptr && canBeHarvested()) ^~~~~~~~~~~~~~~~ apps/openmw/mwrender/bulletdebugdraw.cpp:33:5: warning: Call to virtual method 'DebugDrawer::setDebugMode' during construction bypasses virtual dispatch [clang-analyzer-optin.cplusplus.VirtualCall] setDebugMode(debugMode); ^~~~~~~~~~~~~~~~~~~~~~~ openmw/mwinput/controllermanager.cpp:63:17: warning: Call to virtual method 'ControllerManager::controllerAdded' during construction bypasses virtual dispatch [clang-analyzer-optin.cplusplus.VirtualCall] controllerAdded(fakeDeviceID, evt); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ --- apps/openmw/mwinput/controllermanager.cpp | 2 +- apps/openmw/mwrender/animation.cpp | 2 +- apps/openmw/mwrender/bulletdebugdraw.cpp | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/apps/openmw/mwinput/controllermanager.cpp b/apps/openmw/mwinput/controllermanager.cpp index 4db9fad595..adc62a80c5 100644 --- a/apps/openmw/mwinput/controllermanager.cpp +++ b/apps/openmw/mwinput/controllermanager.cpp @@ -60,7 +60,7 @@ namespace MWInput SDL_ControllerDeviceEvent evt; evt.which = i; static const int fakeDeviceID = 1; - controllerAdded(fakeDeviceID, evt); + ControllerManager::controllerAdded(fakeDeviceID, evt); Log(Debug::Info) << "Detected game controller: " << SDL_GameControllerNameForIndex(i); } else diff --git a/apps/openmw/mwrender/animation.cpp b/apps/openmw/mwrender/animation.cpp index f50ef156b9..0684550fb3 100644 --- a/apps/openmw/mwrender/animation.cpp +++ b/apps/openmw/mwrender/animation.cpp @@ -1838,7 +1838,7 @@ namespace MWRender mObjectRoot->accept(visitor); } - if (ptr.getRefData().getCustomData() != nullptr && canBeHarvested()) + if (ptr.getRefData().getCustomData() != nullptr && ObjectAnimation::canBeHarvested()) { const MWWorld::ContainerStore& store = ptr.getClass().getContainerStore(ptr); if (!store.hasVisibleItems()) diff --git a/apps/openmw/mwrender/bulletdebugdraw.cpp b/apps/openmw/mwrender/bulletdebugdraw.cpp index b169251465..b14b64e771 100644 --- a/apps/openmw/mwrender/bulletdebugdraw.cpp +++ b/apps/openmw/mwrender/bulletdebugdraw.cpp @@ -30,7 +30,7 @@ DebugDrawer::DebugDrawer(osg::ref_ptr parentNode, btCollisionWorld * : mParentNode(parentNode), mWorld(world) { - setDebugMode(debugMode); + DebugDrawer::setDebugMode(debugMode); } void DebugDrawer::createGeometry() From b4f12aace15b53b67047530ce9c0272aa59327b8 Mon Sep 17 00:00:00 2001 From: elsid Date: Wed, 6 Jul 2022 13:13:35 +0200 Subject: [PATCH 7/7] Explicitly ignore result of std::unique_ptr::release call components/nifbullet/bulletnifloader.cpp:206:13: warning: the value returned by this function should be used [bugprone-unused-return-value] boxShape.release(); ^~~~~~~~~~~~~~~~~~ components/nifbullet/bulletnifloader.cpp:232:13: warning: the value returned by this function should be used [bugprone-unused-return-value] child.release(); ^~~~~~~~~~~~~~~ components/nifbullet/bulletnifloader.cpp:233:13: warning: the value returned by this function should be used [bugprone-unused-return-value] mStaticMesh.release(); ^~~~~~~~~~~~~~~~~~~~~ components/nifbullet/bulletnifloader.cpp:240:9: warning: the value returned by this function should be used [bugprone-unused-return-value] mStaticMesh.release(); ^~~~~~~~~~~~~~~~~~~~~ components/nifbullet/bulletnifloader.cpp:246:9: warning: the value returned by this function should be used [bugprone-unused-return-value] mAvoidStaticMesh.release(); ^~~~~~~~~~~~~~~~~~~~~~~~~~ components/nifbullet/bulletnifloader.cpp:411:9: warning: the value returned by this function should be used [bugprone-unused-return-value] childMesh.release(); ^~~~~~~~~~~~~~~~~~~ components/nifbullet/bulletnifloader.cpp:425:9: warning: the value returned by this function should be used [bugprone-unused-return-value] childShape.release(); ^~~~~~~~~~~~~~~~~~~~ --- components/nifbullet/bulletnifloader.cpp | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/components/nifbullet/bulletnifloader.cpp b/components/nifbullet/bulletnifloader.cpp index bedd2ce939..49f5e8f069 100644 --- a/components/nifbullet/bulletnifloader.cpp +++ b/components/nifbullet/bulletnifloader.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include @@ -205,7 +206,7 @@ osg::ref_ptr BulletNifLoader::load(const Nif::File& nif) btTransform transform = btTransform::getIdentity(); transform.setOrigin(center); compound->addChildShape(transform, boxShape.get()); - boxShape.release(); + std::ignore = boxShape.release(); mShape->mCollisionShape.reset(compound.release()); return mShape; @@ -231,21 +232,21 @@ osg::ref_ptr BulletNifLoader::load(const Nif::File& nif) trans.setIdentity(); std::unique_ptr child = std::make_unique(mStaticMesh.get(), true); mCompoundShape->addChildShape(trans, child.get()); - child.release(); - mStaticMesh.release(); + std::ignore = child.release(); + std::ignore = mStaticMesh.release(); } mShape->mCollisionShape = std::move(mCompoundShape); } else if (mStaticMesh != nullptr && mStaticMesh->getNumTriangles() > 0) { mShape->mCollisionShape.reset(new Resource::TriangleMeshShape(mStaticMesh.get(), true)); - mStaticMesh.release(); + std::ignore = mStaticMesh.release(); } if (mAvoidStaticMesh != nullptr && mAvoidStaticMesh->getNumTriangles() > 0) { mShape->mAvoidCollisionShape.reset(new Resource::TriangleMeshShape(mAvoidStaticMesh.get(), false)); - mAvoidStaticMesh.release(); + std::ignore = mAvoidStaticMesh.release(); } return mShape; @@ -410,7 +411,7 @@ void BulletNifLoader::handleNiTriShape(const Nif::NiGeometry& niGeometry, const mCompoundShape.reset(new btCompoundShape); auto childShape = std::make_unique(childMesh.get(), true); - childMesh.release(); + std::ignore = childMesh.release(); float scale = niGeometry.trafo.scale; for (const Nif::Parent* parent = nodeParent; parent != nullptr; parent = parent->mParent) @@ -424,7 +425,7 @@ void BulletNifLoader::handleNiTriShape(const Nif::NiGeometry& niGeometry, const mShape->mAnimatedShapes.emplace(niGeometry.recIndex, mCompoundShape->getNumChildShapes()); mCompoundShape->addChildShape(trans, childShape.get()); - childShape.release(); + std::ignore = childShape.release(); } else if (avoid) fillTriangleMesh(mAvoidStaticMesh, niGeometry, transform);