Details
- Reviewers
- None
- Group Reviewers
Restricted Owners Package (Owns No Changed Paths) - Commits
- rP22210: Const-correct the hierarchical pathfinder.
Compile.
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
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... source/simulation2/helpers/HierarchicalPathfinder.cpp | 1| /*·Copyright·(C)·2016·Wildfire·Games. | | [NORMAL] LicenseYearBear: | | License should have "2019" year instead of "2016" Executing section JS... Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/differential/1230/display/redirect
const-correct
(I guess the previous code was not incorrect, but adding the const adds benefits)
I prefer using .at() here over find() as it makes the code neater and the performance impact is probably negligible.
That's at least an equally important (if not greater) change than adding the const.
The array access inserts an element, but .at throws a std::out_of_range if the element does not exist.
http://www.cplusplus.com/reference/map/map/operator[]/
https://en.cppreference.com/w/cpp/container/map/at
(I guess the elements exist, didn't check further)
Otherwise looks harmless.
Yup, and I think I've actually messed it up but I'll check with an upstream diff.
source/simulation2/helpers/HierarchicalPathfinder.cpp | ||
---|---|---|
564 ↗ | (On Diff #7774) | safe as passClass _should_ always exist (or the code is doing something stupid anyways) |
714 ↗ | (On Diff #7774) | I think this one can crash actually, because there might be no edge to the current region. I might need to change it back. At least it crashes with one of my upstream "outtake" patches (not yet a diff for it) so I'll check. The changes from D53 ensure this doesn't happen so I'll need to back port them here I think: https://code.wildfiregames.com/D53#inline-35820 |
725 ↗ | (On Diff #7774) | safe as passClass _should_ always exist (or the code is doing something stupid anyways) |
740 ↗ | (On Diff #7774) | safe as passClass _should_ always exist (or the code is doing something stupid anyways). |
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... source/simulation2/helpers/HierarchicalPathfinder.cpp | 1| /*·Copyright·(C)·2016·Wildfire·Games. | | [NORMAL] LicenseYearBear: | | License should have "2019" year instead of "2016" Executing section JS... Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/differential/1233/display/redirect
It's probably safer to check whether the key exists (.contains) before possibly running into an exception, and it might be nicer to insert the keys explicitly rather than having them automatically be inserted when trying to read from them (at least if in most cases the key already exists).
source/simulation2/helpers/HierarchicalPathfinder.cpp | ||
---|---|---|
708 ↗ | (On Diff #7777) | .contains? |
.contains is C++20 (though I agree). I don't understand the rest of your message? If the initial region has no edges, it has no neighbours, so we can return early from FindReachableRegions. There's no automatic insertion here?
This is next on my commit list (once builds have passed for the rest) as it is needed for D1832 to cleanly apply, and I'll push new tests proving D1832 then, then commit that, which enables running D1834 on top of all these tests, which I then feel reasonably confident to commit soon (I'd like to run an MP replay with it to ensure hashes are consistent though).