Skip to content
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

WIP warning redux #5260

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Conversation

joonicks
Copy link
Contributor

@joonicks joonicks commented Sep 7, 2021

try to reduce warning spam during compilation

"WIP": while pragma messages are in place to see where warning have been fixed

if PR is accepted the final step would be to clean out all pragma messages

Copy link
Member

@sturnclaw sturnclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In all cases, please do not use bitwise-and with a literal constant, instead please prefer uint32_t(<expr>) (or as needed) as a functional-style cast instead.

@joonicks
Copy link
Contributor Author

Ive been debating with myself which one makes more sense. The resulting code should be identical but the anding version makes more explicit that bit are truncated, a simple typecast looks neater but later on it may be overlooked that it actually truncates bits.

@sturnclaw
Copy link
Member

The lack of visual clarity far outweighs any semantic benefit that might be gained by highlighting the fact that we're truncating the array size to only four billion entries. Please use the functional-style cast, explicit bit constants add visual clutter and make the code harder to read for no benefit in this case.

Copy link
Member

@sturnclaw sturnclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate the effort in fixing all of these warning locations - there are a few tweaks related to the preprocessor that I'd like to see made, but once those are done I'm happy to merge this (with the debug messages removed, of course).

src/galaxy/SystemPath.cpp Outdated Show resolved Hide resolved
src/collider/GeomTree.cpp Show resolved Hide resolved
@joonicks joonicks marked this pull request as draft October 3, 2021 14:08
@impaktor impaktor force-pushed the master branch 3 times, most recently from 876b6fc to b5bc47c Compare July 6, 2022 13:11
@impaktor impaktor force-pushed the master branch 2 times, most recently from 90e4530 to 6a3a044 Compare January 26, 2023 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants