Grid::removeLocation (src/grid.cpp) erases the matching Location from the grid's locations vector without touching the entities that location holds:
void Grid::removeLocation(Location& location) {
std::string targetId = location.getId();
for (auto i = locations.begin(); i != locations.end();) {
if (i->getId() == targetId) {
i = locations.erase(i);
} else {
i++;
}
}
}
Any Entity* held by the erased Location keeps a locationId (set by Location::addEntity, see src/location.cpp) naming a location that is no longer part of the grid. The entity's gridId is likewise left pointing at the grid. Consequences observable through the public surface:
Grid::getNumEntities() silently drops those entities from its total, since it sums over surviving locations only.
Grid::isEntityPresent(entity) returns false while entity.getLocationId() still returns the erased location's id, so the two accessors disagree.
Grid::getLocation(entity.getLocationId()) throws std::runtime_error("Location not found") for an entity that reports itself as located.
By contrast, Grid::removeEntity calls entity.resetLocationId() and sets the grid id to -1, so the intended convention appears to be that an entity leaving the grid has its ids cleared.
Suggested resolution: decide and document the intended semantics — either removeLocation should call resetLocationId() (and clear gridId) on each entity in the location before erasing it, or the method should refuse to remove a non-empty location. Whichever is chosen, a test asserting the resulting entity state should accompany it.
This gap was noticed while adding direct coverage for Grid::removeLocation in PR #35; it is deliberately left out of that PR, which is scoped to the iterator-invalidation fix tracked by #31.
This issue body was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).
drafted by Claude on behalf of Daniel Stephenson
Grid::removeLocation(src/grid.cpp) erases the matchingLocationfrom the grid'slocationsvector without touching the entities that location holds:Any
Entity*held by the erasedLocationkeeps alocationId(set byLocation::addEntity, seesrc/location.cpp) naming a location that is no longer part of the grid. The entity'sgridIdis likewise left pointing at the grid. Consequences observable through the public surface:Grid::getNumEntities()silently drops those entities from its total, since it sums over surviving locations only.Grid::isEntityPresent(entity)returnsfalsewhileentity.getLocationId()still returns the erased location's id, so the two accessors disagree.Grid::getLocation(entity.getLocationId())throwsstd::runtime_error("Location not found")for an entity that reports itself as located.By contrast,
Grid::removeEntitycallsentity.resetLocationId()and sets the grid id to-1, so the intended convention appears to be that an entity leaving the grid has its ids cleared.Suggested resolution: decide and document the intended semantics — either
removeLocationshould callresetLocationId()(and cleargridId) on each entity in the location before erasing it, or the method should refuse to remove a non-empty location. Whichever is chosen, a test asserting the resulting entity state should accompany it.This gap was noticed while adding direct coverage for
Grid::removeLocationin PR #35; it is deliberately left out of that PR, which is scoped to the iterator-invalidation fix tracked by #31.This issue body was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).
drafted by Claude on behalf of Daniel Stephenson