-
-
Notifications
You must be signed in to change notification settings - Fork 300
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
[msvc] Fix warnings, mainly casting to smaller types #270
Conversation
@ntadej — I see the checks passed. Out of curiosity, do the GitHub Actions show a summary of warnings count before and after? And does that number go down with this commit? For iOS builds I checked a sample file that was changed, |
Those warnings are actually silenced at the moment as I found them too spammy (MSVC is also quite verbose). 😊 Also as we have Also to add I runtime tested this on Windows, macOS and iOS (Qt version) and I did not observe any visibly noticeable regressions (I tried really hard not to introduce typos that would affect functionality). |
@roblabs, @atierian, @birkskyum, is there anything else I should do from my side to proceed? |
…aplibre#270)"" This reverts commit 8958d3b.
@@ -122,13 +122,13 @@ IntersectStatus CollisionIndex::intersectsTileEdges(const CollisionBox& box, | |||
const float tileY2 = tileEdges[3]; | |||
|
|||
// Check left border | |||
int minSectionLength = std::min(tileX1 - x1, x2 - tileX1); | |||
float minSectionLength = std::min(tileX1 - x1, x2 - tileX1); |
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.
Seems to break some render tests...
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.
Why this happens is obscure to me. If you look closer there is a cast to int
when this variable is copied over to the result
.
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.
Also it would be useful to actually to paste what tests this breaks to see the effect.
(((p[0] / p[3] + 1) / 2) * size.width) + viewportPadding, | ||
(((-p[1] / p[3] + 1) / 2) * size.height) + viewportPadding | ||
static_cast<float>(((p[0] / p[3] + 1) / 2) * size.width) + viewportPadding, | ||
static_cast<float>(((-p[1] / p[3] + 1) / 2) * size.height) + viewportPadding |
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.
Breaks another render test
@@ -145,7 +145,7 @@ void align(Shaping& shaping, | |||
if (maxLineHeight != lineHeight) { | |||
shiftY = -blockHeight * verticalAlign - Shaping::yOffset; | |||
} else { | |||
shiftY = (-verticalAlign * lineCount + 0.5) * lineHeight; | |||
shiftY = (-verticalAlign * static_cast<float>(lineCount + 0.5)) * lineHeight; |
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.
Was fixed in #285
return util::max(0.0, util::min(xCellCount - 1.0, std::floor(x * xScale))); | ||
return util::max(size_t(0), util::min(xCellCount - 1, static_cast<size_t>(std::floor(x * xScale)))); | ||
} | ||
|
||
template <class T> | ||
std::size_t GridIndex<T>::convertToYCellCoord(const float y) const { | ||
return util::max(0.0, util::min(yCellCount - 1.0, std::floor(y * yScale))); | ||
return util::max(size_t(0), util::min(yCellCount - 1, static_cast<size_t>(std::floor(y * yScale)))); |
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.
Breaks some render tests...
If the checks by GitHub Actions checks are met, then it seems good to go. Thanks for supporting FOSS4G!
MSVC is quite eager with warnings (which is good in my opinion). This MR cleans most of the core codebase of MSVC warnings:
double
tofloat
)Note that this is mostly silencing warnings (e.g. using
static_cast
) but I also tried to fix some trivial cases and add some template specialisations. This should be purely technical, but let's see what also CI says.