Silence VS15 / gcc warnings on rP22817
Details
Details
- Reviewers
- None
- Commits
- rP22877: Silence uninitialized variable warnings on MakeGoalReachable and…
Compile with VS15 or gcc and report that the warnings are gone
Diff Detail
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/52/display/redirect
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/561/display/redirect
Comment Actions
The warnings also appear with gcc in fact:
../../../source/simulation2/helpers/HierarchicalPathfinder.cpp: In member function ‘void HierarchicalPathfinder::FindNearestNavcellInRegions(const std::set<HierarchicalPathfinder::RegionID, HierarchicalPathfinder::SortByCenterToPoint>&, u16&, u16&, pass_class_t) const’:
../../../source/simulation2/helpers/HierarchicalPathfinder.cpp:747:8: warning: ‘bestJ’ may be used uninitialized in this function [-Wmaybe-uninitialized]
747 | jGoal = bestJ;
| ~~~~~~^~~~~~~
../../../source/simulation2/helpers/HierarchicalPathfinder.cpp:746:8: warning: ‘bestI’ may be used uninitialized in this function [-Wmaybe-uninitialized]
746 | iGoal = bestI;
| ~~~~~~^~~~~~~I didn't verify if they are always overwritten otherwise, otherwise thanks for the fix.
Comment Actions
I guess these could indeed be undefined if regions is empty, and that doesn't necessarily sounds like an unrecoverable error, so clarify the comment.
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/87/display/redirect
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/596/display/redirect
Comment Actions
Somewhat sensible default-values
Somewhat sounds like partial. Is there a case where the return value is wrong if empty regions are passed? Like there not being a navcell at the target?
Should there be an early return, perhaps false returned on failure bool on success, or an ENSURE if it should never happen (not even in test setups or the extreme maps)?
Comment Actions
It does sound like we could do something else... But I don't think we can. The two callers of FindNearestNavcellInRegions are MakeGoalReachable and FindNearestPassableNavcell. In both cases, if we have no eligible navels whatsoever (aka the whole map is impassable), then the most sensible thing to do is either crash or do nothing (since the map doesn't make sense anyways). By doing nothing, this behaves exactly as if the whole map was passable then, which sounds fair enough to me.
Reporting the failure doesn't do much good, since there's no way to really recover from that. At the same time it doesn't sound like a crash-worthy offence to me.
I could report a warning, but I'm not sure it'd do much good.