jamoma / JamomaCore

Jamoma Frameworks for Audio and Control Structure
Other
36 stars 14 forks source link

TTElement::round() and truncate() methods are inconsistent #300

Closed nwolek closed 8 years ago

nwolek commented 10 years ago

@tap - please advise on whether this is intentional or a copy/paste error. Once I get your input, I am happy to refactor the code so just assign it back to me. This should probably be a hotfix.

1) round() method is attempting to round integers, which is not possible: https://github.com/jamoma/JamomaCore/blob/issue/165/Foundation/library/includes/TTElement.h#L1049

2) truncate() method is changing the return type which @losius and I agree should not be the case: https://github.com/jamoma/JamomaCore/blob/dev/Foundation/library/includes/TTElement.h#L1045

Note that the trunc() and round() methods in C++ leave type alone. Should we follow this convention? http://www.cplusplus.com/reference/cmath/round/ http://www.cplusplus.com/reference/cmath/trunc/

tap commented 10 years ago

No. 1 is pretty hilarious.

No. 2 is an issue on which I'm uncertain.

It is possible that this is used by @theod , so maybe he has thoughts?

My thoughts: applying a truncation in-place to an existing piece of data is different than calling a function where you get a return value independent of the input variable. Perhaps this could be expressed in the documentation unless there is a more compelling reason to change it?

nwolek commented 10 years ago

Re 1: so not intentional then? :)

Re 2: to bolster the case for leaving type intact -- truncate() and booleanize() are the only ones that change the type. I understand booleanize(), but within this class the following methods also leave type intact:

clip() - https://github.com/jamoma/JamomaCore/blob/issue/165/Foundation/library/includes/TTElement.h#L923

cliplow() - https://github.com/jamoma/JamomaCore/blob/issue/165/Foundation/library/includes/TTElement.h#L965

cliphigh() - https://github.com/jamoma/JamomaCore/blob/issue/165/Foundation/library/includes/TTElement.h#L1007

lossius commented 10 years ago

Hi,

my vote would be that these methods leaves the type intact, the same way that C++ does. In C++ you would e.g. do

double someValue = 3.14 someValue.round

By definition someValue could not change its type.

If I was a C++ programmer coming to Jamoma Core, I'd similarly expect

TTValue myValue = 3,14 myValue.round

to remain a double. Does this make sense?

tap commented 10 years ago

It makes sense, yes. I'd like to see where it used though to ensure that the current behavior is not depended upon. It is likely to be not very many places to look at...

nwolek commented 10 years ago

Makes sense. +1 for keeping type.

--Nathan Wolek

On May 16, 2014, at 3:22 AM, Trond Lossuis notifications@github.com wrote:

Hi,

my vote would be that these methods leaves the type intact, the same way that C++ does. In C++ you would e.g. do

double someValue = 3.14 someValue.round

By definition someValue could not change it types.

If I was a C++ programmer coming to Jamoma Core, I'd expect

TTValue myValue = 3,14 myValue.round

to remain a double. Does this make sense?

— Reply to this email directly or view it on GitHub.

tap commented 8 years ago

Not making potentially hard-to-track bug-inducing changes like this for Jamoma1. In Jamoma2, if we need these types of operations, we should do them as discussed -- returning a copy rather than using a side-effect.