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.
Details
- Reviewers
- None
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 Build Jenkins Build 13201: arc lint + arc unit
Event Timeline
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/1841/display/redirect
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. |
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: |
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. |
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 |
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.