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

Fix a bunch of warnings #57

Merged
merged 17 commits into from
Jul 12, 2024
Merged

Fix a bunch of warnings #57

merged 17 commits into from
Jul 12, 2024

Conversation

illwieckz
Copy link
Member

Fix a bunch of warnings, also delete an unused macro.

@illwieckz illwieckz force-pushed the illwieckz/warn branch 2 times, most recently from 3ce62b6 to 817edf6 Compare July 5, 2024 22:57
Copy link
Member

@slipher slipher left a comment

Choose a reason for hiding this comment

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

crnlib: fix -Wimplicit-const-int-float-conversion looks really ugly. Seems like we'd be better off disabling this warning

I also dislike the crnlib: fix -Wmaybe-uninitialized one. The assignment of dummy values which are never supposed to be used obscures the meaning of the code. Also it prevents sanitizer tools that can detect uninitialized memory use from uncovering possible bugs in the algorithm in the case that a value was unexpectedly not assigned.

crnlib/crn_threading_pthreads.cpp Show resolved Hide resolved
crnlib/crn_file_utils.cpp Outdated Show resolved Hide resolved
crnlib/lzma_LzmaEnc.cpp Outdated Show resolved Hide resolved

#ifdef NDEBUG
#define CRNLIB_ASSUME(p)
#else
#define CRNLIB_ASSUME(p) typedef crnlib_assume_try<sizeof(crnlib_assume_failure<(bool)(p)>)> CRNLIB_JOIN(crnlib_assume_typedef, __COUNTER__)
Copy link
Member

Choose a reason for hiding this comment

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

How about just replacing this with static_assert? CRNLIB_ASSUME is a pre-C++11 implementation for static assertions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just doing this?

#define CRNLIB_ASSUME(p) static_assert(p, "")

Copy link
Member

Choose a reason for hiding this comment

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

yes

Copy link
Member Author

Choose a reason for hiding this comment

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

Now done.

crnlib/lzma_LzmaEnc.cpp Outdated Show resolved Hide resolved
crnlib/crn_miniz.cpp Show resolved Hide resolved
@illwieckz
Copy link
Member Author

crnlib: fix -Wimplicit-const-int-float-conversion looks really ugly. Seems like we'd be better off disabling this warning

This is about warnings like that:

warning: implicit conversion from 'int' to 'float' changes value from 2147483647 to 2147483648
        if (((*m_pVec3F)[component] < INT_MIN) || ((*m_pVec3F)[component] > INT_MAX)) {
                                                                          ~ ^~~~~~~

I guess this MSVC warning spammed in MSVC AppVeyor CI is the same one:

warning C4244: 'argument': conversion from 'T' to 'T', possible loss of data
          with
          [
              T=float
          ]
          and
          [
              T=int
          ]

@illwieckz illwieckz changed the title Fix a bunch of warnings. Fix a bunch of warnings Jul 10, 2024
@slipher
Copy link
Member

slipher commented Jul 10, 2024

crnlib: fix -Wimplicit-const-int-float-conversion looks really ugly. Seems like we'd be better off disabling this warning

This is about warnings like that:

warning: implicit conversion from 'int' to 'float' changes value from 2147483647 to 2147483648
        if (((*m_pVec3F)[component] < INT_MIN) || ((*m_pVec3F)[component] > INT_MAX)) {
                                                                          ~ ^~~~~~~

I guess this MSVC warning spammed in MSVC AppVeyor CI is the same one:

warning C4244: 'argument': conversion from 'T' to 'T', possible loss of data
          with
          [
              T=float
          ]
          and
          [
              T=int
          ]

I looked at that commit closer and it is actually completely wrong. For example in

int i = m_float;
if ((i < INT_MIN) || (i > INT_MAX)) {

The condition is equivalent to if (false). An int cannot be less than INT_MIN or greater than INT_MAX

@illwieckz
Copy link
Member Author

The condition is equivalent to if (false). An int cannot be less than INT_MIN or greater than INT_MAX

Right, this is stupid indeed!

I rewrote that commit by doing (float)INT_MAX instead.

@@ -39,15 +39,7 @@ struct crnlib_assume_failure<true> {
template <int x>
struct crnlib_assume_try {};
Copy link
Member

Choose a reason for hiding this comment

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

More CRNLIB_ASSUME bits that can be deleted above

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@slipher
Copy link
Member

slipher commented Jul 12, 2024

As I said earlier I don't think it's an improvement to treat Wmaybe-uninitialized. LGTM to the other commits

@illwieckz
Copy link
Member Author

As I said earlier I don't think it's an improvement to treat Wmaybe-uninitialized. LGTM to the other commits

Thanks, I now removed this one.

@illwieckz illwieckz merged commit d9481b5 into master Jul 12, 2024
14 checks passed
@illwieckz illwieckz deleted the illwieckz/warn branch July 12, 2024 02:33
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