Page MenuHomeWildfire Games

Avoid VertexPathfinder pointer in rP22253
Needs ReviewPublic

Authored by elexis on Jun 29 2019, 2:43 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

rP22253 moved the VertexPathfinder to a separate file and uses a pointer.
Splitting itself seems reasonable, but I didn't read the entire commit, so I can't judge that.
In general it is preferable to avoid pointers where possible to avoid null-dereferencing and leaks.

Test Plan

Test the assumption of this diff to fix instead of revert rP22253 by auditing that commit and seeing if the class architecture should be different.
Consider whether passing the two variables is cleaner than passing an instance to the CCmpPathfinder and adding getters for these two variables, or otherwise simple refactoring.
Notice that rP22278 introduces two more of these pointers which might or might not be addressed similarly.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8111
Build 13202: Vulcan BuildJenkins
Build 13201: arc lint + arc unit

Event Timeline

elexis created this revision.Jun 29 2019, 2:43 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/1841/display/redirect

elexis added inline comments.Jun 29 2019, 2:49 PM
source/simulation2/helpers/VertexPathfinder.cpp
513

One could also pass mapSize * Pathfinding::NAVCELLS_PER_TILE and should pass a reference to terrainOnlyGrid instead of a pointer. I don't really want to work on it, more communicate that one shouldn't opt in for a pointer when there is no need to.

wraitii added a subscriber: wraitii.EditedJun 29 2019, 3:03 PM

Notes:

  • This will make the pathfinder component have a bigger memory footprint, which is probably not a problem but I didn't super-like.
  • This prevents the forward-include, which I dislike.
  • I originally made this a pointer to work around the issue of passing arguments in the constructor, since component constructors aren't accessible. Passing them in function calls is sufficient to work around that and is possibly a better solution regardless of the pointer debate.

The usage of pointers in rP22278 was for consistency so if this goes in, the others must be changed too.

source/simulation2/helpers/VertexPathfinder.cpp
513

Note:
A reference to the object itself couldn't be passed when this was kept in the VertexPathfinder class because the object being pointed-to could change, and we would have a dangling reference.
Passing it in the function makes it possible to use a const-ref directly there.

vladislavbelov added inline comments.
source/simulation2/helpers/VertexPathfinder.cpp
513

Grid<NavcellData>* const& terrainOnlyGrid seems strange, why that? Usually it's useless to pass pointers by references. The only case when the pointer can be changed during the function, but it'd even stranger as it's const.

wraitii added inline comments.Jun 29 2019, 8:03 PM
source/simulation2/helpers/VertexPathfinder.cpp
513

It's merely because I had a const reference to a pointer in the class (which was weird but made sense) and elexis didn't change that

elexis added a comment.Jul 2 2019, 5:15 PM

Either the VertexPathfinder uses a pointer, then it would IMO be cleaner to add nullderef safeguards, or one avoids the pointer and adds a Reset() function in the VertexPathfinder to clean the cache.
In both cases one has to check that every content is present when needed and deleted when unneeded.