fix of implicit type conversion.

xiph / vorbis

Reference implementation of the Ogg Vorbis audio format.

BSD 3-Clause "New" or "Revised" License

467 stars 188 forks source link

fix of implicit type conversion. #79

Open ihsinme opened 3 years ago

ihsinme commented 3 years ago

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 commented 3 years ago

good day. somebody will look at my PR.

petterreinholdtsen commented 3 years ago

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

ihsinme commented 3 years ago

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 commented 3 years ago

is this PR still relevant?

ihsinme commented 3 years ago

I have to do something to make this PR useful.

petterreinholdtsen commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

[ihsinme]

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?

I am not the maintainer of vorbis, so I do not really have an opinion on this.

-- Happy hacking Petter Reinholdtsen

rillian commented 3 years ago

@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?

rillian commented 3 years ago

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

ihsinme commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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

ihsinme commented 2 years ago

is there any news on this PR?