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 clang warning due to implicit conversion from int to float for the RAND_MAX macro #7717

Merged
merged 2 commits into from
Mar 1, 2025

Conversation

Rossmaxx
Copy link
Contributor

@Rossmaxx Rossmaxx commented Feb 16, 2025

A minor warning, and a minor fix. RAND_MAX is an integer macro, which seems to introduce a warning when divided.

@firewall1110
Copy link
Contributor

It seems You have fixed not all such warnings:

/plugins/Organic/Organic.cpp:239:18:
/plugins/Organic/Organic.cpp:241:18:
/plugins/Vibed/VibratingString.h:110:49:
/plugins/Vibed/VibratingString.h:116:49:
/plugins/Vibed/VibratingString.h:127:50:
/plugins/Vibed/VibratingString.h:136:50:
/plugins/Vibed/VibratingString.cpp:81:48:

floatwarnings.txt

P.S.
I am using clang compiler

@Rossmaxx
Copy link
Contributor Author

@firewall1110 thanks for the catches. I just noted the one case which was giving me trouble. I'll fix the rest later, but I'll take some time off so can't really say when.

@tresf tresf marked this pull request as draft February 18, 2025 17:43
@tresf tresf added the rework required Pull requests which must be reworked to make it able to merge or review label Feb 18, 2025
@Rossmaxx
Copy link
Contributor Author

@firewall1110 can you check again?

@Rossmaxx
Copy link
Contributor Author

Rossmaxx commented Feb 22, 2025

I am using clang compiler

Ahh yes, now i remember. I was using clang for iwyu which is when i saw these warnings. After that, I was on gcc, and thought the warnings disappeared.

@Rossmaxx Rossmaxx removed the rework required Pull requests which must be reworked to make it able to merge or review label Feb 22, 2025
@Rossmaxx Rossmaxx changed the title Fix compiler warning due to implicit conversion from int to float Fix clang warning due to implicit conversion from int to float for the RAND_MAX macro Feb 22, 2025
@Rossmaxx Rossmaxx marked this pull request as ready for review February 22, 2025 09:31
@firewall1110
Copy link
Contributor

@firewall1110 can you check again?

After ~5 h - now I about to go to rehearsal with vocal ensemble ...

@firewall1110
Copy link
Contributor

There is no more RAND_MAX related warnings.

P.S.
There are more clang different type warnings ...

Compilation stderr:
err_log.txt

Used build script:

export CC=clang
export CXX=clang++
export CFLAGS="-Oz"
export CXXFLAGS="-Oz"

cmake  .. -DCMAKE_INSTALL_PREFIX=../clang_Oz/

make -j6

Invocation command: bash build_clang_Oz.sh 2>err_log.txt

@Rossmaxx
Copy link
Contributor Author

@firewall1110 you fine if i get done with just the RAND_MAX warnings. I'll look at the other warnings seperately.

@firewall1110
Copy link
Contributor

firewall1110 commented Feb 23, 2025

i get done with just the RAND_MAX warnings. I'll look at the other warnings seperately.

File in P.S. is only for complete information, all others warnings should not be fixed here.

[Edited:] You respect me too much (Thank You for this, but ... )

@Rossmaxx
Copy link
Contributor Author

@sakertooth @messmerd can i get one approval and get this merged real quick?

@@ -163,7 +163,7 @@ class LMMS_EXPORT Oscillator

static inline sample_t noiseSample( const float )
{
return 1.0f - rand() * 2.0f / RAND_MAX;
return 1.0f - rand() * 2.0f / static_cast<float>(RAND_MAX);
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it would be worth adding a range version for rand like @rubiefawn did for fastRand:

lmms/include/lmms_math.h

Lines 88 to 99 in 3417dfe

template<std::floating_point T>
inline auto fastRand(T range) noexcept
{
constexpr T FAST_RAND_RATIO = static_cast<T>(1.0 / 32767);
return fastRand() * range * FAST_RAND_RATIO;
}
template<std::floating_point T>
inline auto fastRand(T from, T to) noexcept
{
return from + fastRand(to - from);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That might make sense too, but I am focusing more on #7677 rn so this has to wait a bit, if i'm refactoring

Copy link
Contributor

@rubiefawn rubiefawn Mar 1, 2025

Choose a reason for hiding this comment

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

There's a lot of places in the code where rand()/std::rand() is used in this way, which we may want to change to just use fastRand(). I would argue that belongs in a separate PR since it is beyond the scope of "fix compiler warnings".

Edit: See #7741

@messmerd messmerd merged commit 050df38 into LMMS:master Mar 1, 2025
10 checks passed
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.

6 participants