-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
typified a little bit of map.h + dependents #76340
Conversation
I only noticed this PR after #76623, which caused a small conflict that hopefully shouldn't be too hard to fix. Now with #76671 there are even more chances for conflicts, or even code errors if they're merged independently, so I'll probably leave that in draft until this one is merged.
Interestingly my VS tells me that's an error, but still compiles it without issues. The fix isn't wrong, though. |
@mqrause: Merge conflicts happen all the time with this. Both because the tentacles spread all over the place, and thus bump into a lot of stuff, and because it often takes a lot of time for these PRs to be merged. I haven't looked at your new PR yet, but I think in general it should be sufficient to provide a warning against concurrent merging. If it's anything like the previous typification PR it should be rather contained and quickly reviewed and merged, while this PR may well linger for another month. Finally, thanks for looking at the template issue. It's definitely helpful to get a competent assessment of such issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, just minor comments that don't really require action.
But as expected there are a few minor cases where this and #76671 will clash, so they definitely can't be merged together. Should be easy fixes for whoever gets merged last, though.
Note that we just had a rare case of no tests failing. Just noting this, as it's unlikely to remain the case after updating based on the review. |
Summary
None
Purpose of change
A baby step converting usage of untyped coordinates into typed ones.
Still working on map.h and dependents.
Edit:
Fix #76336. Correction slipped in inadvertently (failed to clear it out after investigation). Note that the misapplied type part still remains, but it can be cleaned up when the operation is typified (probably the next PR), and it has no technical impact.
Describe the solution
Convert/duplicate untyped operations into corresponding types ones and converting usages to the typed one when it can be done with reasonable ease.
Ended up with a tangent into testing code... Has to be done eventually regardless.
Note that it should result in no functional change.
Edit: That turned out not to be correct. A correction to #76336 happened to remain in the code as I missed to remove it. However, that correction should be done, but I don't know how we'd be able to detect or test it.
Describe alternatives you've considered
Testing
Loaded a save, walked out of a basement, into a car, drove a bit through hay bales. Nothing odd seen.
Additional context
Another goose chase after template messes suddenly deciding it couldn't resolve unchanged code. A theory is that inclusion of coords_fwd.h in some header that might have been included transitively caused it to suddenly fail to resolve types (or it could just be pure spite...). This time it was possible to nail down the type to use by specifying it (although it should change to a typed one eventually, but that pain is for another day).
If anyone is interested, the mess was caused by visitable_zone_test.cpp operation place_structures, where "rectangle" decided it wasn't resolved any longer.