rolling container refactoring+tests & promise refactoring

[Pospelove]

Important

Refactored Promise and RollingContainer with new storage policies, added bounds checking, and introduced comprehensive tests for RollingContainer.

• Promise Refactoring:
  • Introduced SharedStorage and InlineStorage policies in Promise.h.
  • Added Forward() method to BasicPromise for forwarding results to another promise.
  • Modified All() and Race() methods to work only with SharedStorage.
  • Added MoveOnlyPromise alias for InlineStorage.
• RollingContainer Refactoring:
  • Added BoundsChecks enum and DefaultTerminator in RollingContainer.h.
  • Implemented bounds checking and overflow handling using static_assert.
  • Added ForgetAll() method to reset the container state.
• Tests:
  • Added RollingContainerTest.cpp with tests for insertion, access, iteration, and overflow behavior.
  • Tests include scenarios with and without bounds checks, using CheckedContainer and UncheckedContainer.

^{This description was created by for 643d24b. You can customize this summary. It will automatically update as commits are pushed.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed everything up to 9e83c07 in 1 minute and 31 seconds. Click for details.

• Reviewed 404 lines of code in 4 files
• Skipped 0 files when reviewing.
• Skipped posting 5 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. skymp5-server/cpp/server_guest_lib/script_classes/PapyrusObjectReference.cpp:673

• Draft comment:
  Refactored to use Promise::Race() instead of 'Any'. This change makes the intent (racing a timer and listener promises) clearer. Confirm that 'Race' semantics are as expected.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is asking the PR author to confirm the behavior of Promise::Race() when used with a timer and listener promises. It is asking for confirmation of expected behavior, which violates the rule against asking the author to confirm their intention or ensure behavior is intended.

2. viet/include/RollingContainer.h:38

• Draft comment:
  The static_assert ensures bounds checking is enabled for a custom (smaller) MaxLogicalIndex. Make sure ROLLING_CONTAINER_DISABLE_STATIC_ASSERT is only used in tests, not in production.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is purely informative and does not provide a specific code suggestion or request for clarification. It violates the rule against making purely informative comments.

3. viet/include/RollingContainer.h:55

• Draft comment:
  Operator[] computes the offset correctly and conditionally terminates on out-of-bounds access. Consider adding const versions of Begin() and End() for completeness.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

4. viet/include/Promise.h:101

• Draft comment:
  The Promise::Race() refactoring is clear. Note that lambdas capture 'res' by value, which is acceptable due to shared_ptr semantics, though capturing by reference might clarify intent.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 20% <= threshold 50% The comment is informative, explaining the behavior of lambdas capturing by value and suggesting capturing by reference for clarity. It doesn't ask for a change or suggest a specific improvement, so it doesn't align with the rules for useful comments.

5. unit/RollingContainerTest.cpp:5

• Draft comment:
  The macro ROLLING_CONTAINER_DISABLE_STATIC_ASSERT is enabled intentionally here for testing unchecked behavior. Ensure this macro is confined to test environments.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

Workflow ID: wflow_4dSOGY53dEEy8kIw

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 643d24b in 2 minutes and 11 seconds. Click for details.

• Reviewed 286 lines of code in 4 files
• Skipped 0 files when reviewing.
• Skipped posting 5 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. viet/include/Promise.h:89

• Draft comment:
  In BasicPromise::Then and Catch the callback is simply overwritten, so multiple calls won’t accumulate callbacks. Confirm this is the intended design or consider storing multiple callbacks.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment is about behavior that existed before this PR - the old code also overwrote callbacks. This isn't a change introduced by this PR. The comment is also phrased as "Confirm this is the intended design" which violates the rule about not asking the PR author to confirm their intention or explain things. The comment doesn't point to a definite bug, it's speculative ("or consider storing multiple callbacks"). This type of comment asking for confirmation or suggesting alternative designs without pointing to a concrete problem should be deleted according to the rules. Perhaps the refactoring in this PR makes this behavior more visible or important to reconsider? Maybe the introduction of the storage policies or the move from const methods to non-const methods changes the semantics in a way that makes callback accumulation more relevant? Even if the refactoring makes this more visible, the fundamental behavior of overwriting callbacks hasn't changed. The comment is still asking the author to "confirm" rather than pointing out a definite issue. The rules explicitly state not to ask the PR author to confirm their intention or explain things. This is pre-existing behavior, not a bug introduced by the changes. This comment should be deleted. It's commenting on pre-existing behavior (callback overwriting existed before this PR), and it's asking the author to "confirm" the design rather than pointing out a concrete issue. This violates the rule against asking the PR author to confirm their intention.

2. viet/include/Promise.h:101

• Draft comment:
  The lambda functions in Forward capture the promise by value. Ensure that this copy‐based capture is acceptable for your promise type (especially for non‐copyable, move‐only promises) or consider capturing by reference if safe.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 15% vs. threshold = 50% The comment says "Ensure that this copy-based capture is acceptable" and "consider capturing by reference if safe." This is asking the author to verify/ensure something, which violates the rule about not asking the PR author to confirm their intention or double-check things. Additionally, the comment is somewhat speculative - it says "especially for non-copyable, move-only promises" but doesn't definitively state there's a problem. In fact, the code seems designed to handle this: move-only promises can't be copied, so if you try to use Forward with them, it will fail at compile time. This might be intentional design - Forward only works with copyable promises. The comment doesn't provide a clear, actionable fix - it just asks the author to "ensure" and "consider" things. Perhaps the comment is pointing out a legitimate design issue where Forward should support move-only promises but currently doesn't. The author might have intended Forward to work with both copyable and move-only promises, and this is a bug that needs fixing by changing the capture mechanism. While that's possible, the comment doesn't definitively state there's a bug - it uses language like "ensure" and "consider", which are red flags per the rules. If this were truly a bug, the comment should say something like "This won't compile for move-only promises, change to capture by reference" or provide a concrete fix. The speculative nature ("if safe") and the ask for the author to verify their intention makes this not actionable enough to keep. The comment is asking the author to "ensure" and "consider" things rather than pointing out a definite issue with a clear fix. It's speculative and violates the rule about not asking the PR author to confirm or verify their intentions. It should be deleted.

3. skymp5-server/cpp/server_guest_lib/script_classes/PapyrusUtility.cpp:191

• Draft comment:
  Consider writing the comparison as 'if (seconds < 0)' instead of 'if (0.0 > seconds)' for clarity.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

4. skymp5-server/cpp/server_guest_lib/script_classes/PapyrusUtility.cpp:229

• Draft comment:
  In ArrayHelper, when resizing (if 'resize' is true), arguments[0].pArray is dereferenced without an explicit null check. Ensure that this pointer is valid or add appropriate error/validation handling.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

5. skymp5-server/cpp/server_guest_lib/script_classes/PapyrusUtility.cpp:162

• Draft comment:
  Assignment to the global 'compatibilityPolicy' (set in Register) might have thread-safety implications. Confirm that this global is protected in multi-threaded scenarios.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50% None

Workflow ID: wflow_okO5J3lUMYgFbdsP

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}