Unify counter clamp arithmetic into shared addClamped() helper

- Add addClamped() in new header clamped_arithmetic.h; uses a 64-bit
  intermediate so the addition cannot overflow int.
- Use it in Server_Card::incrementCounter() (clamps [0, MAX_COUNTERS_ON_CARD])
  and Server_Counter::incrementCount() (clamps [INT_MIN, INT_MAX]), removing
  the duplicated overflow-safe logic and its keep-in-sync TODO.
- Inline incrementCount() into server_counter.h; server_counter.cpp now holds
  only the constructor and getInfo().
- Clarify the card-counter bounds comment in trice_limits.h.
This commit is contained in:
DawnFire42 2026-06-19 12:20:50 -04:00
parent fcac7493ad
commit 7aed233bd8
No known key found for this signature in database
GPG key ID: 24BB855EE2911B33
6 changed files with 39 additions and 17 deletions

View file

@ -26,6 +26,7 @@
#include <libcockatrice/protocol/pb/event_set_card_attr.pb.h> #include <libcockatrice/protocol/pb/event_set_card_attr.pb.h>
#include <libcockatrice/protocol/pb/event_set_card_counter.pb.h> #include <libcockatrice/protocol/pb/event_set_card_counter.pb.h>
#include <libcockatrice/protocol/pb/serverinfo_card.pb.h> #include <libcockatrice/protocol/pb/serverinfo_card.pb.h>
#include <libcockatrice/utility/clamped_arithmetic.h>
#include <libcockatrice/utility/trice_limits.h> #include <libcockatrice/utility/trice_limits.h>
#include <limits> #include <limits>
@ -139,10 +140,8 @@ bool Server_Card::setCounter(int _id, int value, Event_SetCardCounter *event)
bool Server_Card::incrementCounter(int counterId, int delta, Event_SetCardCounter *event) bool Server_Card::incrementCounter(int counterId, int delta, Event_SetCardCounter *event)
{ {
const int oldValue = counters.value(counterId, 0); const int oldValue = counters.value(counterId, 0);
const auto result = static_cast<int64_t>(oldValue) + static_cast<int64_t>(delta);
// Clamp to [0, MAX_COUNTERS_ON_CARD] for card counters // Clamp to [0, MAX_COUNTERS_ON_CARD] for card counters
const int newValue = const int newValue = addClamped(oldValue, delta, 0, MAX_COUNTERS_ON_CARD);
static_cast<int>(qBound(static_cast<int64_t>(0), result, static_cast<int64_t>(MAX_COUNTERS_ON_CARD)));
if (newValue == oldValue) { if (newValue == oldValue) {
return false; return false;

View file

@ -1,24 +1,12 @@
#include "server_counter.h" #include "server_counter.h"
#include <libcockatrice/protocol/pb/serverinfo_counter.pb.h> #include <libcockatrice/protocol/pb/serverinfo_counter.pb.h>
#include <limits>
Server_Counter::Server_Counter(int _id, const QString &_name, const color &_counterColor, int _radius, int _count) 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) : id(_id), name(_name), counterColor(_counterColor), radius(_radius), count(_count)
{ {
} }
//! \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<int64_t>(count) + static_cast<int64_t>(delta);
count = static_cast<int>(qBound(static_cast<int64_t>(std::numeric_limits<int>::min()), result,
static_cast<int64_t>(std::numeric_limits<int>::max())));
return count != oldCount;
}
void Server_Counter::getInfo(ServerInfo_Counter *info) void Server_Counter::getInfo(ServerInfo_Counter *info)
{ {
info->set_id(id); info->set_id(id);

View file

@ -22,6 +22,8 @@
#include <QString> #include <QString>
#include <libcockatrice/protocol/pb/color.pb.h> #include <libcockatrice/protocol/pb/color.pb.h>
#include <libcockatrice/utility/clamped_arithmetic.h>
#include <limits>
class ServerInfo_Counter; class ServerInfo_Counter;
@ -92,7 +94,12 @@ public:
* @return true if the value changed, false otherwise. * @return true if the value changed, false otherwise.
* @note Clamps result to [INT_MIN, INT_MAX] to prevent overflow. * @note Clamps result to [INT_MIN, INT_MAX] to prevent overflow.
*/ */
[[nodiscard]] bool incrementCount(int delta); [[nodiscard]] bool incrementCount(int delta)
{
const int oldCount = count;
count = addClamped(count, delta, std::numeric_limits<int>::min(), std::numeric_limits<int>::max());
return count != oldCount;
}
/** /**
* @brief Populates info with this counter's current state for network serialization. * @brief Populates info with this counter's current state for network serialization.

View file

@ -16,6 +16,7 @@ set(UTILITY_HEADERS
libcockatrice/utility/macros.h libcockatrice/utility/macros.h
libcockatrice/utility/passwordhasher.h libcockatrice/utility/passwordhasher.h
libcockatrice/utility/trice_limits.h libcockatrice/utility/trice_limits.h
libcockatrice/utility/clamped_arithmetic.h
libcockatrice/utility/zone_names.h libcockatrice/utility/zone_names.h
libcockatrice/utility/days_years_between.h libcockatrice/utility/days_years_between.h
) )

View file

@ -0,0 +1,22 @@
#ifndef CLAMPED_ARITHMETIC_H
#define CLAMPED_ARITHMETIC_H
#include <QtGlobal>
#include <cstdint>
/**
* @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.
*
* @note Requires minValue <= maxValue. Bounds come from trusted compile-time call sites;
* qBound() asserts this internally in debug builds.
*/
inline int addClamped(int value, int delta, int minValue, int maxValue)
{
const auto result = static_cast<int64_t>(value) + static_cast<int64_t>(delta);
return static_cast<int>(qBound(static_cast<int64_t>(minValue), result, static_cast<int64_t>(maxValue)));
}
#endif // CLAMPED_ARITHMETIC_H

View file

@ -1,6 +1,9 @@
#ifndef TRICE_LIMITS_H #ifndef TRICE_LIMITS_H
#define TRICE_LIMITS_H #define TRICE_LIMITS_H
//! \todo Split trice_limits.h into focused single-purpose headers: string_limits.h,
//! dice_limits.h, counter_limits.h.
#include <QString> #include <QString>
// max size for short strings, like names and things that are generally a single phrase // max size for short strings, like names and things that are generally a single phrase
@ -16,7 +19,9 @@ constexpr uint MINIMUM_DICE_TO_ROLL = 1;
constexpr uint MAXIMUM_DICE_TO_ROLL = 100; constexpr uint MAXIMUM_DICE_TO_ROLL = 100;
// Card counter value bounds [0, MAX_COUNTERS_ON_CARD]. // 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. // This caps an individual counter's VALUE (e.g. a +1/+1 counter at 999), not how many counters a card holds.
// Applies to card counters only; player counters (Server_Counter) are unbounded and may go
// negative (e.g. life total), saturating only at the int range.
// The max of 999 is a display constraint (3-digit rendering) and reasonable gameplay limit. // 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. // Server enforces these bounds; client may also check for UX optimization.
constexpr int MAX_COUNTERS_ON_CARD = 999; constexpr int MAX_COUNTERS_ON_CARD = 999;