Skip to content

Comments

Small perf: one less unneeded allocation#3280

Closed
VariableVince wants to merge 1 commit intomainfrom
set-alloc-unnecessary
Closed

Small perf: one less unneeded allocation#3280
VariableVince wants to merge 1 commit intomainfrom
set-alloc-unnecessary

Conversation

@VariableVince
Copy link
Contributor

@VariableVince VariableVince commented Feb 22, 2026

Description:

(retracted)

Please complete the following:

  • I have added screenshots for all UI updates
  • I process any text displayed to the user through translateText() and I've added it to the en.json file
  • I have added relevant tests to the test directory
  • I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced

Please put your Discord username so you can be contacted if a bug or regression is found:

tryout33

…st get this._tiles which is a set, and the method turns it into a ReadOnlySet already
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 22, 2026

Walkthrough

The tiles() method in PlayerImpl was modified to return the internal _tiles Set directly as a ReadonlySet<TileRef> instead of constructing a new Set from _tiles.values(). This eliminates defensive copying while maintaining the public API contract.

Changes

Cohort / File(s) Summary
PlayerImpl tiles() method
src/core/game/PlayerImpl.ts
Modified tiles() to return internal _tiles Set directly instead of creating a new Set. Changes return behavior from new Set(_tiles.values()) to _tiles as ReadonlySet<TileRef>.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~5 minutes

Poem

A set returned, no copy made,
Direct exposure, bold cascade,
Internal tiles now bare and free,
Defensive walls give way to speed. 📦✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: eliminating an unnecessary Set allocation in the tiles() method for performance improvement.
Description check ✅ Passed The description clearly explains why the change was made and references the specific method and performance improvement, directly relating to the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/core/game/PlayerImpl.ts`:
- Around line 334-336: Revert the change to tiles(): it must return a defensive
snapshot rather than the live Set to avoid iterator invalidation; update
PlayerImpl.tiles() to return a new Set constructed from this._tiles (e.g., new
Set(this._tiles.values())) so callers like SpawnExecution
(player.tiles().forEach(...)) and tests that iterate other.tiles() won’t be
iterating the internal _tiles while GameImpl.relinquish() or GameImpl.conquer()
call _tiles.delete(); also audit borderTiles() for similar live-alias risks and
make it return a snapshot if it currently returns the internal Set.

@github-project-automation github-project-automation bot moved this from Triage to Development in OpenFront Release Management Feb 22, 2026
@github-project-automation github-project-automation bot moved this from Development to Complete in OpenFront Release Management Feb 22, 2026
@VariableVince VariableVince deleted the set-alloc-unnecessary branch February 22, 2026 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Performance Performance optimization

Projects

Status: Complete

Development

Successfully merging this pull request may close these issues.

1 participant