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 of implicit type conversion. #79

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

ihsinme
Copy link

@ihsinme ihsinme commented Dec 29, 2020

in casting used in your code, first the result of division is converted to an integer type, with the loss of the fractional part, and then the integer is converted to a floating point type. I think this can be simply fixed.

@ihsinme
Copy link
Author

ihsinme commented Jan 13, 2021

good day.
somebody will look at my PR.

@petterreinholdtsen
Copy link
Contributor

Apparently the floor fix broke something. Btw, did you consider divinding by 2.0 instead of casting the value?

@ihsinme
Copy link
Author

ihsinme commented Feb 5, 2021

sorry for the long answer.
in fact, division by 2.0 is also a solution.
but I recommend using more explicit things that persist after refactoring.)
if you insist I will fix it to 2.0.

@ihsinme
Copy link
Author

ihsinme commented Feb 16, 2021

is this PR still relevant?

@ihsinme
Copy link
Author

ihsinme commented May 17, 2021

I have to do something to make this PR useful.

@petterreinholdtsen
Copy link
Contributor

Note, https://gitlab.xiph.org/xiph/vorbis/ is the upstream project, I've been told the github repository is a convenience copy. Perhaps it is an idea to submit the patch there?

@ihsinme
Copy link
Author

ihsinme commented May 17, 2021

Oh no. I don't have an account there. maybe a better idea would be if you fix the code there yourself.
do I close PR?

@petterreinholdtsen
Copy link
Contributor

petterreinholdtsen commented May 17, 2021 via email

@rillian
Copy link
Contributor

rillian commented May 19, 2021

@ihsinme Thanks for the patches. Can you say a bit more about what issue you're trying to address? Do you think this is a correctness problem? Are you trying to address warnings from a particular compiler?

@@ -106,7 +106,7 @@ static int _ve_amp(envelope_lookup *ve,
/* stretch is used to gradually lengthen the number of windows
considered prevoius-to-potential-trigger */
int stretch=max(VE_MINSTRETCH,ve->stretch/2);
float penalty=gi->stretch_penalty-(ve->stretch/2-VE_MINSTRETCH);
float penalty=gi->stretch_penalty-((float)(ve->stretch)/2-VE_MINSTRETCH);
Copy link
Contributor

Choose a reason for hiding this comment

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

The stretch itself is calculated with integer division, so maybe that's a more accurate heuristic for the penalty?

Copy link
Author

Choose a reason for hiding this comment

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

you know better

@rillian
Copy link
Contributor

rillian commented May 19, 2021

The appveyor integration test failure is an unrelated issue fixed in 894d1b1. A rebase should complete without error.

@ihsinme
Copy link
Author

ihsinme commented May 21, 2021

@ihsinme Thanks for the patches. Can you say a bit more about what issue you're trying to address? Do you think this is a correctness problem? Are you trying to address warnings from a particular compiler?

the essence of the error is described in PR. this is an integer division, when a fraction is expected.

@rillian
Copy link
Contributor

rillian commented Jun 4, 2021

Just because the assigned variable is floating point doesn't mean the calculations must all be floating point. We are free to choose at what points integer inputs are converted. For example:

  • Floating point operations can preserve more precision in calculations.
  • Integer operations can be faster than floating point.
  • Floating point operations can be faster than checking for integer overflow.
  • Division by zero has different results based on the type of the operands.

Which of these criteria are most important varies. That's why I asked what specific issue you wanted to address.

@ihsinme
Copy link
Author

ihsinme commented Jun 4, 2021

Good day.
I see an error that the division will be integer.

@ihsinme
Copy link
Author

ihsinme commented Feb 11, 2022

is there any news on this PR?

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.

3 participants