CODEWITHSUNDEEP
c++
morgancodes
morgancodes

Reputation: 25265

c++ method sometimes returns unexpected high values

I've traced a bug down to a function which should be returning float values between 20 and 100 or so, but is sometimes (1 time in 10) returning values much much higher than that. The problem exists when I have an expression in the last line of the method, like this:

return snap(baseNumber, targets) + (octave * NOTES_PER_OCTAVE);

If I store the return value in a variable first, then return that variable, the problem goes away:

float ret = snap(baseNumber, targets) + (octave * NOTES_PER_OCTAVE);
return ret;

Here's the complete method:

static inline float octaveSnap(float number, std::vector<float>* targets){
    static const int NOTES_PER_OCTAVE = 12;
    int octave = number / NOTES_PER_OCTAVE;
    float baseNumber = number - (octave * NOTES_PER_OCTAVE);
    float ret = snap(baseNumber, targets) + (octave * NOTES_PER_OCTAVE);
    return ret;
}

and here's 'snap':

// given a single value and a list of values (a scale), return the member of the list which is closest to the single value 
static inline float snap(float number, std::vector<float>* targets){
    float ret;
    float leastDistance = -1;
    for(int i = 0; i<targets->size(); i++){
        float distance = targets->at(i) - number;
        if(distance < 0){
            distance = -distance;
        }
        if(leastDistance == -1){
            leastDistance = distance;
        }
        if(distance < leastDistance){
            leastDistance = distance;
            ret = targets->at(i);
        }
    }
    return ret;
}

I'm completely baffled by this. Any idea why the first explodes and the second works perfectly?

Upvotes: 3

Views: 616

Answers (2)

uesp
uesp

Reputation: 6204

In snap() the local variable ret is never initialized so if the input vector is either zero-sized or the "found" element is the first one then your return value is unspecified.

Try modifying snap to be:

static inline float snap(float number, std::vector<float>* targets){
    float ret = 0;
    float leastDistance = -1;
    for(int i = 0; i<targets->size(); i++){
        float distance = targets->at(i) - number;
        if(distance < 0){
            distance = -distance;
        }
        if(leastDistance == -1){
            leastDistance = distance;
            ret = targets->at(i);
        }
        else if(distance < leastDistance){
            leastDistance = distance;
            ret = targets->at(i);
        }
    }
    return ret;
}

and see if that fixes things.

Edit: I realized this doesn't address why adding a temporary variable appears to fix things in the original question. The uninitialized ret will probably take on whatever value is left on the stack: this, of course, is unspecified and system/platform dependent. When a new local variable is added to store the result of snap(), however, this shifts the stack such that ret has a different position, most likely, a different uninitialized value. The return result is still "wrong" but it may simply appear "less wrong" due to whatever uninitialized value ret has.

Upvotes: 3

Mark B
Mark B

Reputation: 96281

My psychic debugging powers tell me that when you use the temp variable the problem only appears to go away and that either you're accidentally doing targets[<foo>] inside snap or you use it correctly but rarely run off the end, returning garbage.

EDIT for comment:

I should elaborate a bit: targets is a pointer to vector so using [] on it will select one of several vectors, NOT elements from the vector. That said I can't understand how you could call .at on such a pointer, so I suspect the code in your program is not the code you showed us.

Upvotes: 4

Related Questions