diff --git a/cockatrice/src/game/player/player_actions.cpp b/cockatrice/src/game/player/player_actions.cpp index fffd23ccf..fd1c16e39 100644 --- a/cockatrice/src/game/player/player_actions.cpp +++ b/cockatrice/src/game/player/player_actions.cpp @@ -1527,9 +1527,9 @@ void PlayerActions::offsetCardCounter(QList selectedCards, int count int oldValue = card->getCounters().value(counterId, 0); int newValue = oldValue + offset; - // Early exit optimization: server enforces [0, MAX_COUNTERS_ON_CARD]. + // Early exit optimization: server enforces [0, MAX_COUNTER_VALUE]. // Compare clamped value to allow recovery from invalid states. - int clampedValue = qBound(0, newValue, MAX_COUNTERS_ON_CARD); + int clampedValue = qBound(0, newValue, MAX_COUNTER_VALUE); if (clampedValue != oldValue) { auto *cmd = new Command_SetCardCounter; cmd->set_zone(card->getZone()->getName().toStdString()); @@ -1563,7 +1563,7 @@ void PlayerActions::actSetCardCounter(QList selectedCards, int count Expression exp(oldValue); double parsed = exp.parse(counterValue); // Clamp in double precision first to avoid UB, then cast - int number = static_cast(qBound(0.0, parsed, static_cast(MAX_COUNTERS_ON_CARD))); + int number = static_cast(qBound(0.0, parsed, static_cast(MAX_COUNTER_VALUE))); auto *cmd = new Command_SetCardCounter; cmd->set_zone(card->getZone()->getName().toStdString()); @@ -1593,7 +1593,7 @@ void PlayerActions::actIncrementAllCardCounters(QList cardsToUpdate) counterIterator.next(); int counterId = counterIterator.key(); int currentValue = counterIterator.value(); - if (currentValue >= MAX_COUNTERS_ON_CARD) { + if (currentValue >= MAX_COUNTER_VALUE) { continue; } diff --git a/libcockatrice_network/libcockatrice/network/server/remote/game/server_card.cpp b/libcockatrice_network/libcockatrice/network/server/remote/game/server_card.cpp index b858314c0..fdd44485c 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/game/server_card.cpp +++ b/libcockatrice_network/libcockatrice/network/server/remote/game/server_card.cpp @@ -114,8 +114,8 @@ QString Server_Card::setAttribute(CardAttribute attribute, const QString &avalue bool Server_Card::setCounter(int _id, int value, Event_SetCardCounter *event) { - // Clamp to valid card counter range [0, MAX_COUNTERS_ON_CARD] - value = qBound(0, value, MAX_COUNTERS_ON_CARD); + // Clamp to valid card counter range [0, MAX_COUNTER_VALUE] + value = qBound(0, value, MAX_COUNTER_VALUE); const int oldValue = counters.value(_id, 0); if (value == oldValue) { @@ -139,10 +139,8 @@ bool Server_Card::setCounter(int _id, int value, Event_SetCardCounter *event) bool Server_Card::incrementCounter(int counterId, int delta, Event_SetCardCounter *event) { const int oldValue = counters.value(counterId, 0); - const auto result = static_cast(oldValue) + static_cast(delta); - // Clamp to [0, MAX_COUNTERS_ON_CARD] for card counters - const int newValue = - static_cast(qBound(static_cast(0), result, static_cast(MAX_COUNTERS_ON_CARD))); + // Clamp to [0, MAX_COUNTER_VALUE] for card counters + const int newValue = addClamped(oldValue, delta, 0, MAX_COUNTER_VALUE); if (newValue == oldValue) { return false; diff --git a/libcockatrice_network/libcockatrice/network/server/remote/game/server_card.h b/libcockatrice_network/libcockatrice/network/server/remote/game/server_card.h index 3d7e649b9..a2698ad61 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/game/server_card.h +++ b/libcockatrice_network/libcockatrice/network/server/remote/game/server_card.h @@ -156,7 +156,7 @@ public: /** * @brief Sets a card counter to an exact value with clamping. * @param _id The counter ID. - * @param value The desired value (clamped to [0, MAX_COUNTERS_ON_CARD]; 0 removes the counter). + * @param value The desired value (clamped to [0, MAX_COUNTER_VALUE]; 0 removes the counter). * @param event Optional event to populate with counter state. * @return true if the value changed, false otherwise. */ @@ -168,7 +168,7 @@ public: * @param event Optional event to populate with counter state. * @return true if the value changed, false otherwise. * @note If counter does not exist, starts from 0. Counter is removed if result is 0. - * @note Clamps result to [0, MAX_COUNTERS_ON_CARD]. + * @note Clamps result to [0, MAX_COUNTER_VALUE]. */ [[nodiscard]] bool incrementCounter(int counterId, int delta, Event_SetCardCounter *event = nullptr); void setTapped(bool _tapped) diff --git a/libcockatrice_network/libcockatrice/network/server/remote/game/server_counter.cpp b/libcockatrice_network/libcockatrice/network/server/remote/game/server_counter.cpp index e65205cbb..2138ac9ca 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/game/server_counter.cpp +++ b/libcockatrice_network/libcockatrice/network/server/remote/game/server_counter.cpp @@ -1,24 +1,19 @@ #include "server_counter.h" #include -#include -Server_Counter::Server_Counter(int _id, const QString &_name, const color &_counterColor, int _radius, int _count) - : id(_id), name(_name), counterColor(_counterColor), radius(_radius), count(_count) +Server_Counter::Server_Counter(int _id, + const QString &_name, + const color &_counterColor, + int _radius, + int _count, + int _minValue, + int _maxValue) + : id(_id), name(_name), counterColor(_counterColor), radius(_radius), count(_count), minValue(_minValue), + maxValue(_maxValue) { } -//! \todo Extract overflow-safe arithmetic into shared helper. -//! Duplicated in Server_Card::incrementCounter() - keep in sync if modified. -bool Server_Counter::incrementCount(int delta) -{ - const int oldCount = count; - const auto result = static_cast(count) + static_cast(delta); - count = static_cast(qBound(static_cast(std::numeric_limits::min()), result, - static_cast(std::numeric_limits::max()))); - return count != oldCount; -} - void Server_Counter::getInfo(ServerInfo_Counter *info) { info->set_id(id); diff --git a/libcockatrice_network/libcockatrice/network/server/remote/game/server_counter.h b/libcockatrice_network/libcockatrice/network/server/remote/game/server_counter.h index 8226e663f..ac6574e87 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/game/server_counter.h +++ b/libcockatrice_network/libcockatrice/network/server/remote/game/server_counter.h @@ -22,18 +22,19 @@ #include #include +#include +#include class ServerInfo_Counter; /** * @class Server_Counter - * @brief Represents a player counter with overflow-safe increment arithmetic. + * @brief Represents a player counter with overflow-safe increment arithmetic and optional bounds. * * All value modifications return whether the value actually changed, * enabling callers to skip unnecessary network events. * - * @note Direct assignment via setCount() does not clamp; only - * incrementCount() enforces int boundary saturation. + * @note Values are clamped to [minValue, maxValue] on both setCount() and incrementCount(). * @note Unlike card counters, player counters are never auto-removed * when they reach zero - they persist with value 0. */ @@ -45,9 +46,29 @@ protected: color counterColor; int radius; int count; + int minValue; ///< Minimum allowed value (default: INT_MIN, i.e. unbounded) + int maxValue; ///< Maximum allowed value (default: INT_MAX, i.e. unbounded) + + static constexpr int DEFAULT_MAX_VALUE = std::numeric_limits::max(); public: - Server_Counter(int _id, const QString &_name, const color &_counterColor, int _radius, int _count = 0); + /** + * @brief Constructs a counter. + * @param _id Unique counter identifier + * @param _name Display name + * @param _counterColor Counter color + * @param _radius Display radius + * @param _count Initial value (default 0) + * @param _minValue Minimum allowed value (default INT_MIN) + * @param _maxValue Maximum allowed value (default INT_MAX) + */ + Server_Counter(int _id, + const QString &_name, + const color &_counterColor, + int _radius, + int _count = 0, + int _minValue = std::numeric_limits::min(), + int _maxValue = DEFAULT_MAX_VALUE); ~Server_Counter() { } @@ -73,16 +94,15 @@ public: } /** - * @brief Sets the counter to an exact value. - * @param _count The new value (assigned directly without clamping). - * @return true if the value changed, false otherwise. - * @warning This performs raw assignment. For overflow-safe incrementing, - * use incrementCount(). + * @brief Sets the counter value, clamping to [minValue, maxValue]. + * @param _count The desired new value + * @return true if the clamped value differs from the previous value + * @note For increment operations, prefer incrementCount() which handles overflow safely. */ [[nodiscard]] bool setCount(int _count) { - const int oldCount = count; - count = _count; + int oldCount = count; + count = qBound(minValue, _count, maxValue); return count != oldCount; } @@ -90,9 +110,14 @@ public: * @brief Increments the counter by delta with overflow-safe arithmetic. * @param delta The amount to add (may be negative for decrement). * @return true if the value changed, false otherwise. - * @note Clamps result to [INT_MIN, INT_MAX] to prevent overflow. + * @note Clamps result to [minValue, maxValue] to prevent overflow. */ - [[nodiscard]] bool incrementCount(int delta); + [[nodiscard]] bool incrementCount(int delta) + { + const int oldCount = count; + count = addClamped(count, delta, minValue, maxValue); + return count != oldCount; + } /** * @brief Populates info with this counter's current state for network serialization. diff --git a/libcockatrice_utility/libcockatrice/utility/trice_limits.h b/libcockatrice_utility/libcockatrice/utility/trice_limits.h index 833ce1b98..a50dea301 100644 --- a/libcockatrice_utility/libcockatrice/utility/trice_limits.h +++ b/libcockatrice_utility/libcockatrice/utility/trice_limits.h @@ -2,6 +2,8 @@ #define TRICE_LIMITS_H #include +#include +#include // max size for short strings, like names and things that are generally a single phrase constexpr int MAX_NAME_LENGTH = 0xff; @@ -15,11 +17,23 @@ constexpr uint MAXIMUM_DIE_SIDES = 1000000; constexpr uint MINIMUM_DICE_TO_ROLL = 1; constexpr uint MAXIMUM_DICE_TO_ROLL = 100; -// Card counter value bounds [0, MAX_COUNTERS_ON_CARD]. -// Counters on cards (e.g., +1/+1 counters, charge counters) are non-negative physical game objects. +// Counter value bounds [0, MAX_COUNTER_VALUE]. +// Counters (on cards or players) are non-negative values. // The max of 999 is a display constraint (3-digit rendering) and reasonable gameplay limit. // Server enforces these bounds; client may also check for UX optimization. -constexpr int MAX_COUNTERS_ON_CARD = 999; +constexpr int MAX_COUNTER_VALUE = 999; + +/** + * @brief Overflow-safe clamped addition: returns value + delta bounded to [minValue, maxValue]. + * + * Uses a 64-bit intermediate so the addition itself cannot overflow int. Shared by the + * counter arithmetic in Server_Card and Server_Counter so both stay in sync. + */ +inline int addClamped(int value, int delta, int minValue, int maxValue) +{ + const auto result = static_cast(value) + static_cast(delta); + return static_cast(qBound(static_cast(minValue), result, static_cast(maxValue))); +} // optimized functions to get qstrings that are at most that long static inline QString nameFromStdString(const std::string &_string) diff --git a/tests/server_card_counter_test.cpp b/tests/server_card_counter_test.cpp index ff906b906..b6aacc31b 100644 --- a/tests/server_card_counter_test.cpp +++ b/tests/server_card_counter_test.cpp @@ -28,9 +28,9 @@ TEST(ServerCardCounter, IncrementExistingCounter) TEST(ServerCardCounter, IncrementOverflowProtection) { Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0); - ASSERT_TRUE(card.setCounter(1, MAX_COUNTERS_ON_CARD)); + ASSERT_TRUE(card.setCounter(1, MAX_COUNTER_VALUE)); EXPECT_FALSE(card.incrementCounter(1, 1)); - EXPECT_EQ(card.getCounter(1), MAX_COUNTERS_ON_CARD); + EXPECT_EQ(card.getCounter(1), MAX_COUNTER_VALUE); } TEST(ServerCardCounter, DecrementUnderflowProtection) @@ -113,13 +113,13 @@ TEST(ServerCardCounter, IncrementCounterPopulatesEvent) TEST(ServerCardCounter, IncrementCounterEventReflectsClampedValue) { Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0); - ASSERT_TRUE(card.setCounter(1, MAX_COUNTERS_ON_CARD - 5)); + ASSERT_TRUE(card.setCounter(1, MAX_COUNTER_VALUE - 5)); Event_SetCardCounter event; EXPECT_TRUE(card.incrementCounter(1, 10, &event)); EXPECT_EQ(event.counter_id(), 1); - EXPECT_EQ(event.counter_value(), MAX_COUNTERS_ON_CARD); + EXPECT_EQ(event.counter_value(), MAX_COUNTER_VALUE); } TEST(ServerCardCounter, IncrementCounterNoEventWhenNullptr) @@ -133,7 +133,7 @@ TEST(ServerCardCounter, IncrementCounterNoEventWhenNullptr) TEST(ServerCardCounter, IncrementCounterEventNotPopulatedWhenUnchanged) { Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0); - ASSERT_TRUE(card.setCounter(1, MAX_COUNTERS_ON_CARD)); + ASSERT_TRUE(card.setCounter(1, MAX_COUNTER_VALUE)); Event_SetCardCounter event; event.set_counter_id(999); @@ -156,7 +156,7 @@ TEST(ServerCardCounter, SetCounterClampsAboveMaxToMax) { Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0); EXPECT_TRUE(card.setCounter(1, 1500)); - EXPECT_EQ(card.getCounter(1), MAX_COUNTERS_ON_CARD); + EXPECT_EQ(card.getCounter(1), MAX_COUNTER_VALUE); } TEST(ServerCardCounter, IncrementDoesNotGoBelowZero) @@ -171,9 +171,9 @@ TEST(ServerCardCounter, IncrementDoesNotGoBelowZero) TEST(ServerCardCounter, IncrementDoesNotExceedMax) { Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0); - ASSERT_TRUE(card.setCounter(1, MAX_COUNTERS_ON_CARD - 5)); + ASSERT_TRUE(card.setCounter(1, MAX_COUNTER_VALUE - 5)); EXPECT_TRUE(card.incrementCounter(1, 10)); - EXPECT_EQ(card.getCounter(1), MAX_COUNTERS_ON_CARD); + EXPECT_EQ(card.getCounter(1), MAX_COUNTER_VALUE); } int main(int argc, char **argv) diff --git a/tests/server_counter_test.cpp b/tests/server_counter_test.cpp index 0f41f2cbd..148a646ee 100644 --- a/tests/server_counter_test.cpp +++ b/tests/server_counter_test.cpp @@ -5,6 +5,7 @@ #include #include +#include #include TEST(ServerCounter, IncrementDoesNotOverflow) @@ -79,6 +80,37 @@ TEST(ServerCounter, MixedExtremesDoNotClamp) EXPECT_EQ(c.getCount(), -1); } +TEST(ServerCounter, SetCountClampsToCustomBounds) +{ + Server_Counter c(1, "test", color(), 10, 50, 0, 100); + EXPECT_TRUE(c.setCount(150)); + EXPECT_EQ(c.getCount(), 100); + EXPECT_TRUE(c.setCount(-10)); + EXPECT_EQ(c.getCount(), 0); +} + +TEST(ServerCounter, IncrementClampsToCustomBounds) +{ + Server_Counter c(1, "test", color(), 10, 50, 0, 100); + EXPECT_TRUE(c.incrementCount(100)); + EXPECT_EQ(c.getCount(), 100); + EXPECT_FALSE(c.incrementCount(1)); + EXPECT_EQ(c.getCount(), 100); + EXPECT_TRUE(c.incrementCount(-200)); + EXPECT_EQ(c.getCount(), 0); + EXPECT_FALSE(c.incrementCount(-1)); + EXPECT_EQ(c.getCount(), 0); +} + +TEST(ServerCounter, CustomBoundsClampToMaxCounterValue) +{ + Server_Counter c(1, "test", color(), 20, 0, 0, MAX_COUNTER_VALUE); + EXPECT_TRUE(c.setCount(1000)); + EXPECT_EQ(c.getCount(), MAX_COUNTER_VALUE); + EXPECT_TRUE(c.setCount(-5)); + EXPECT_EQ(c.getCount(), 0); +} + int main(int argc, char **argv) { ::testing::InitGoogleTest(&argc, argv);