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

android/sound.cpp: Fix overflow warning #1385

Closed
wants to merge 1 commit into from

Conversation

softins
Copy link
Member

@softins softins commented Mar 29, 2021

Hopefully, this should fix the overflow warning that gets listed below every PR.

@softins softins marked this pull request as draft March 29, 2021 16:17
@hoffie
Copy link
Member

hoffie commented Mar 29, 2021

Hrm, Android build is currently failing:

android/sound.cpp:329:24: error: no matching function for call to 'min'
    int32_t count    = std::min ( mOutBuffer.size(), to_write );
                       ^~~~~~~~
/opt/android/android-ndk/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/c++/v1/algorithm:2532:1: note: candidate template ignored: deduced conflicting types for parameter '_Tp' ('unsigned int' vs. 'int')
min(const _Tp& __a, const _Tp& __b)
^
/opt/android/android-ndk/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/c++/v1/algorithm:2543:1: note: candidate template ignored: could not match 'initializer_list<type-parameter-0-0>' against 'unsigned int'
min(initializer_list<_Tp> __t, _Compare __comp)
^
/opt/android/android-ndk/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/c++/v1/algorithm:2523:1: note: candidate function template not viable: requires 3 arguments, but 2 were provided
min(const _Tp& __a, const _Tp& __b, _Compare __comp)
^
/opt/android/android-ndk/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/c++/v1/algorithm:2552:1: note: candidate function template not viable: requires single argument '__t', but 2 arguments were provided
min(initializer_list<_Tp> __t)

@softins
Copy link
Member Author

softins commented Mar 29, 2021

Hrm, Android build is currently failing:

Yes, I noticed, but I can’t understand why. I’m trying to set up a build environment, based on the workflow file, but haven’t succeeded yet.

@pljones
Copy link
Collaborator

pljones commented Mar 31, 2021

Yes, I noticed, but I can’t understand why. I’m trying to set up a build environment, based on the workflow file, but haven’t succeeded yet.

Fork and reduce the builds done on your fork to just the failing one (and then down to just the needed bits, if that's easy enough) -- or is that what you meant?

@softins
Copy link
Member Author

softins commented Apr 1, 2021

Yes, I noticed, but I can’t understand why. I’m trying to set up a build environment, based on the workflow file, but haven’t succeeded yet.

Fork and reduce the builds done on your fork to just the failing one (and then down to just the needed bits, if that's easy enough) -- or is that what you meant?

Well that's certainly one way to do it. I had been hoping to create a local build environment to mirror what Github does, but didn't succeed.

@ann0see
Copy link
Member

ann0see commented Jun 24, 2021

Todo: fix error on Android. @j-santander might have an idea?

android/sound.cpp:329:24: error: no matching function for call to 'min'
100
int32_t count = std::min ( mOutBuffer.size(), to_write );
101
^~~~~~~~
102
/opt/android/android-ndk/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/c++/v1/algorithm:2532:1: note: candidate template ignored: deduced conflicting types for parameter '_Tp' ('unsigned int' vs. 'int')
103
min(const _Tp& __a, const _Tp& __b)
104
^
105
/opt/android/android-ndk/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/c++/v1/algorithm:2543:1: note: candidate template ignored: could not match 'initializer_list' against 'unsigned int'
106
min(initializer_list<_Tp> __t, _Compare __comp)
107
^
108
/opt/android/android-ndk/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/c++/v1/algorithm:2523:1: note: candidate function template not viable: requires 3 arguments, but 2 were provided
109
min(const _Tp& __a, const _Tp& __b, _Compare __comp)
110
^
111
/opt/android/android-ndk/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/c++/v1/algorithm:2552:1: note: candidate function template not viable: requires single argument '__t', but 2 arguments were provided
112
min(initializer_list<_Tp> __t)
113
^

@j-santander
Copy link
Contributor

j-santander commented Jun 24, 2021

Fails because you have unsigned (the return of the ring buffer size) and signed arguments (the count) in the std::min

what's this overflow warning? Tried to check the logs of recent builds but either I don't know how to check it or it wasn't there.

@j-santander
Copy link
Contributor

BTW, I did a PR #1528 to replace my custom ring buffer with the CBuffer common to the rest of the project.

@pljones
Copy link
Collaborator

pljones commented Nov 13, 2021

  1. Is this still a problem?
  2. Is the patch still clean?
  3. Is the Android build still failing?

@softins
Copy link
Member Author

softins commented Nov 13, 2021

  1. Is this still a problem?

I expect so, and I believe there are others too. I have been intending to look at them all, but not got there yet.

  1. Is the patch still clean?

Don't know.

  1. Is the Android build still failing?

Will check when I pick it up again.

@ann0see
Copy link
Member

ann0see commented Jan 27, 2022

Closing in favour of #2292

@ann0see ann0see closed this Jan 27, 2022
@hoffie
Copy link
Member

hoffie commented Jan 27, 2022

Closing in favour of #2292

Hrm, isn't this about two different issues?
This PR touches android/sound.cpp, while #2292 touches src/buffer.cpp so it sounds different to me at first glance?

Edit: Maybe we should put even more context in PR/commit titles to simplify distinguishing. :)

@hoffie hoffie changed the title Fix overflow warning android/sound.cpp: Fix overflow warning Jan 27, 2022
@ann0see
Copy link
Member

ann0see commented Jan 27, 2022

Android should already be fixed?

#1528 #2237

@hoffie
Copy link
Member

hoffie commented Jan 27, 2022

Ah, right. This should be the relevant fix:
https://github.com/jamulussoftware/jamulus/pull/2237/files#diff-32ef006d0b2b2daef9b1dc9c1e43c991a5b41822a1a5ef972592c6d1b636ca48R299

I was just confused by the seemingly unrelated mention of #2292.

@softins softins deleted the fix-warnings branch August 31, 2023 15:18
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.

5 participants