Skip to content

Grid::addEntityToLocation inserts into the caller's Location argument instead of the grid's own element #43

Description

@dmccoystephenson

Grid::addEntityToLocation (src/grid.cpp:82-89) walks locations looking for an element whose id matches the location argument, but when a match is found the entity is pushed into the argument rather than into the matching grid-owned element:

for (Location& l : locations) {
    if (l.getId() == location.getId()) {
        location.addEntity(&entity);   // `l` is the grid's element; `location` may not be
    }
}

The loop variable l is never used for the insertion. Two consequences follow:

  1. A caller-owned Location with a matching id receives the entity while the grid's own copy stays empty. Grid::addLocation stores a copy of its argument (locations.push_back(location), src/grid.cpp:54), so the pattern in testAddingLocationToGrid — construct a local Location, pass it to addLocation, keep using the local — produces exactly this situation. After grid.addEntityToLocation(entity, localLocation), entity.getLocationId() and entity.getGridId() both report the grid, yet grid.isEntityPresent(entity) returns false and grid.getNumEntities() is unchanged, because the pointer was stored in the local copy.

  2. When no id matches, the entity's grid id is still stamped. entity.setGridId(id) runs on line 83 before the membership check, so an entity that was never added anywhere is left reporting membership of the grid (and, via Environment::addEntityToLocation, of the environment), with getLocationId() still "N/S".

The existing test for this path (testAddingEntityToSpecificLocation, Test 29) does not catch either case because it obtains its Location& from getGrid()->getFirstLocation(), i.e. a reference to the grid's own element, where the argument and l are the same object.

Suggested fix: insert into l instead of location (l.addEntity(&entity);), and move entity.setGridId(id) inside the match branch (or return early / throw when nothing matches, mirroring getLocation's std::runtime_error("Location not found")). A regression test should add a location via addLocation, then call addEntityToLocation with the caller's original object and assert grid.isEntityPresent(entity).

This issue body was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).


drafted by Claude on behalf of Daniel Stephenson

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions