From 8e73dfd4405972b380cd6ec1fb3a0a3967f22a46 Mon Sep 17 00:00:00 2001 From: Vinicius Sa Date: Fri, 2 Oct 2020 13:41:19 -0300 Subject: [PATCH 1/7] Fix possible memory leak in CreateObjectFromJson As reported by PVS-Studio. Warning: V773. Fix 1/7. Issue: 12523 --- src/openrct2/object/ObjectFactory.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/openrct2/object/ObjectFactory.cpp b/src/openrct2/object/ObjectFactory.cpp index 15138c417d..37974b72a4 100644 --- a/src/openrct2/object/ObjectFactory.cpp +++ b/src/openrct2/object/ObjectFactory.cpp @@ -472,6 +472,7 @@ namespace ObjectFactory result->ReadJson(&readContext, jRoot); if (readContext.WasError()) { + delete result; throw std::runtime_error("Object has errors"); } From b93024039ab3b4a919833aaf0554317f10366302 Mon Sep 17 00:00:00 2001 From: Vinicius Sa Date: Fri, 2 Oct 2020 13:53:23 -0300 Subject: [PATCH 2/7] Remove redundant sub-expression involving or bitwise Reported by PVS-Studio (Warning: V501). Fix 2/7. Issue: 12523 --- src/openrct2-ui/windows/Cheats.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/openrct2-ui/windows/Cheats.cpp b/src/openrct2-ui/windows/Cheats.cpp index 8d9e96ff2a..72168ebe86 100644 --- a/src/openrct2-ui/windows/Cheats.cpp +++ b/src/openrct2-ui/windows/Cheats.cpp @@ -400,7 +400,6 @@ static uint64_t window_cheats_page_enabled_widgets[] = { (1ULL << WIDX_DAY_BOX) | (1ULL << WIDX_DAY_UP) | (1ULL << WIDX_DAY_DOWN) | - (1ULL << WIDX_MONTH_BOX) | (1ULL << WIDX_DATE_GROUP) | (1ULL << WIDX_DATE_RESET), From d583911997d70ba03b21c58e10a0c0414ecc1ac0 Mon Sep 17 00:00:00 2001 From: Vinicius Sa Date: Sat, 3 Oct 2020 00:32:06 -0300 Subject: [PATCH 3/7] Avoid variable shadowing in derived struct RCT12BannerElement Give a more meaningful name to its private member that caused the shadowing, as well as in the struct BannerElement. Reported by PVS-Studio (Warning: V703). Fix 3/7. Issue: 12523 --- src/openrct2/rct12/RCT12.cpp | 6 +++--- src/openrct2/rct12/RCT12.h | 6 +++--- src/openrct2/world/Banner.cpp | 8 ++++---- src/openrct2/world/TileElement.h | 4 ++-- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/openrct2/rct12/RCT12.cpp b/src/openrct2/rct12/RCT12.cpp index 8fd2606d75..5032753e08 100644 --- a/src/openrct2/rct12/RCT12.cpp +++ b/src/openrct2/rct12/RCT12.cpp @@ -473,7 +473,7 @@ uint8_t RCT12BannerElement::GetPosition() const uint8_t RCT12BannerElement::GetAllowedEdges() const { - return flags & 0b00001111; + return AllowedEdges & 0b00001111; } bool is_user_string_id(rct_string_id stringId) @@ -990,8 +990,8 @@ void RCT12BannerElement::SetPosition(uint8_t newPosition) void RCT12BannerElement::SetAllowedEdges(uint8_t newEdges) { - flags &= ~0b00001111; - flags |= (newEdges & 0b00001111); + AllowedEdges &= ~0b00001111; + AllowedEdges |= (newEdges & 0b00001111); } bool RCT12ResearchItem::IsInventedEndMarker() const diff --git a/src/openrct2/rct12/RCT12.h b/src/openrct2/rct12/RCT12.h index b94b2e3824..39e1212413 100644 --- a/src/openrct2/rct12/RCT12.h +++ b/src/openrct2/rct12/RCT12.h @@ -589,9 +589,9 @@ assert_struct_size(RCT12EntranceElement, 8); struct RCT12BannerElement : RCT12TileElementBase { private: - uint8_t index; // 4 - uint8_t position; // 5 - uint8_t flags; // 6 + uint8_t index; // 4 + uint8_t position; // 5 + uint8_t AllowedEdges; // 6 #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wunused-private-field" uint8_t unused; // 7 diff --git a/src/openrct2/world/Banner.cpp b/src/openrct2/world/Banner.cpp index dd864b6ce3..fd78e3ac0d 100644 --- a/src/openrct2/world/Banner.cpp +++ b/src/openrct2/world/Banner.cpp @@ -379,18 +379,18 @@ void BannerElement::SetPosition(uint8_t newPosition) uint8_t BannerElement::GetAllowedEdges() const { - return flags & 0b00001111; + return AllowedEdges & 0b00001111; } void BannerElement::SetAllowedEdges(uint8_t newEdges) { - flags &= ~0b00001111; - flags |= (newEdges & 0b00001111); + AllowedEdges &= ~0b00001111; + AllowedEdges |= (newEdges & 0b00001111); } void BannerElement::ResetAllowedEdges() { - flags |= 0b00001111; + AllowedEdges |= 0b00001111; } Banner* GetBanner(BannerIndex id) diff --git a/src/openrct2/world/TileElement.h b/src/openrct2/world/TileElement.h index f27618366b..95767ac0e7 100644 --- a/src/openrct2/world/TileElement.h +++ b/src/openrct2/world/TileElement.h @@ -532,8 +532,8 @@ private: uint8_t position; // 5 #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wunused-private-field" - uint8_t flags; // 6 - uint8_t unused; // 7 + uint8_t AllowedEdges; // 6 + uint8_t unused; // 7 uint8_t pad_09[7]; #pragma clang diagnostic pop public: From 59e9162137e6a39779e58132e811d3f90f4912fc Mon Sep 17 00:00:00 2001 From: Vinicius Sa Date: Sat, 3 Oct 2020 00:51:26 -0300 Subject: [PATCH 4/7] Give a variable for an arithmetic operation used as a condition Avoid reverse logic around if-statement for the sake of readability. Reported by PVS-Studio (Warning: V793). Fix 4/7. Issue: 12523 --- src/openrct2/ride/gentle/ObservationTower.cpp | 21 ++++++++++++------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/openrct2/ride/gentle/ObservationTower.cpp b/src/openrct2/ride/gentle/ObservationTower.cpp index bcf3ad1f56..a11c8b936f 100644 --- a/src/openrct2/ride/gentle/ObservationTower.cpp +++ b/src/openrct2/ride/gentle/ObservationTower.cpp @@ -35,18 +35,23 @@ void vehicle_visual_observation_tower( int32_t baseImage_id = (vehicle->restraints_position / 64); if (vehicle->restraints_position >= 64) { - if ((imageDirection / 8) && (imageDirection / 8) != 3) + auto directionOffset = imageDirection / 8; + if ((directionOffset == 0) || (directionOffset == 3)) { - baseImage_id *= 2; - baseImage_id += vehicleEntry->base_image_id + 28; - if ((imageDirection / 8) != 1) - { - baseImage_id -= 6; - } + baseImage_id = vehicleEntry->base_image_id + 8; } else { - baseImage_id = vehicleEntry->base_image_id + 8; + baseImage_id *= 2; + baseImage_id += vehicleEntry->base_image_id; + if (directionOffset == 1) + { + baseImage_id += 28; + } + else + { + baseImage_id += 22; + } } } else From 579e008590a1d9e5875bfaf706ed10ee03989fe0 Mon Sep 17 00:00:00 2001 From: Vinicius Sa Date: Fri, 2 Oct 2020 22:21:32 -0300 Subject: [PATCH 5/7] Refactor process_mouse_over() - Remove unnecessary local variables and assignments - Remove unreached if-statement branch Reported by PVS-Studio (Warning: V587). Fix 5/7. Issue: 12523 --- src/openrct2-ui/input/MouseInput.cpp | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/src/openrct2-ui/input/MouseInput.cpp b/src/openrct2-ui/input/MouseInput.cpp index 204e6148d7..64f7b800b8 100644 --- a/src/openrct2-ui/input/MouseInput.cpp +++ b/src/openrct2-ui/input/MouseInput.cpp @@ -1089,8 +1089,6 @@ void process_mouse_over(const ScreenCoordsXY& screenCoords) if (window != nullptr) { - int32_t ebx, edi; - rct_window* subWindow; rct_widgetindex widgetId = window_find_widget_from_point(window, screenCoords); if (widgetId != -1) { @@ -1107,21 +1105,6 @@ void process_mouse_over(const ScreenCoordsXY& screenCoords) break; } cursorId = gCurrentToolId; - subWindow = window_find_by_number( - gCurrentToolWidget.window_classification, gCurrentToolWidget.window_number); - if (subWindow == nullptr) - break; - - ebx = 0; - edi = cursorId; - // Window event WE_UNKNOWN_0E was called here, but no windows actually implemented a handler and - // it's not known what it was for - cursorId = edi; - if ((ebx & 0xFF) != 0) - { - set_cursor(cursorId); - return; - } break; case WWT_FRAME: From c6b7893d778a6332ea3d3aaf55b579b92e179d9f Mon Sep 17 00:00:00 2001 From: Vinicius Sa Date: Fri, 2 Oct 2020 22:27:46 -0300 Subject: [PATCH 6/7] Verify pointer against nullptr before using it Reported by PVS-Studio (Warning: V1004). Fix 6/7. Issue: 12523 --- src/openrct2/network/NetworkBase.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/openrct2/network/NetworkBase.cpp b/src/openrct2/network/NetworkBase.cpp index 30dc0f4557..c1c1b48b8a 100644 --- a/src/openrct2/network/NetworkBase.cpp +++ b/src/openrct2/network/NetworkBase.cpp @@ -1852,9 +1852,8 @@ void NetworkBase::ProcessPlayerList() { _serverConnection->Player = player; } + newPlayers.push_back(player->Id); } - - newPlayers.push_back(player->Id); } else { From 22a1df32d742ba139b7ccd28964ab4a2dbf1506a Mon Sep 17 00:00:00 2001 From: Vinicius Sa Date: Fri, 2 Oct 2020 22:37:02 -0300 Subject: [PATCH 7/7] Remove redundant assignment in CustomListView::MouseUp Reported by PVS-Studio (Warning: V1048). Fix 7/7. Issue: 12523 --- src/openrct2-ui/scripting/CustomListView.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/openrct2-ui/scripting/CustomListView.cpp b/src/openrct2-ui/scripting/CustomListView.cpp index 23598d2e62..a4e8e87dc5 100644 --- a/src/openrct2-ui/scripting/CustomListView.cpp +++ b/src/openrct2-ui/scripting/CustomListView.cpp @@ -527,7 +527,6 @@ void CustomListView::MouseUp(const ScreenCoordsXY& pos) if (!ColumnHeaderPressedCurrentState) { ColumnHeaderPressed = std::nullopt; - ColumnHeaderPressedCurrentState = false; Invalidate(); } }