Can't find my memory leak in this simple code

Question

I was 100% positive that I covered all ground in terms of deleting memory from the heap before it was lost, but valgrind seems to disagree. Any help with finding the leak in the following code would be greatly appreciated! I can't seem to figure out what's causing it

                    Card * S = new Card[subsetSize];
                    Card * M = nullptr;
                    int subsetPrice = 0, subsetProfit = 0;
                    for(int i = 0; i < subsetSize; i++){
                            S[i] = problemCards[cardIndexesToAdd[i]];
                            subsetPrice += S[i].getPrice();
                            subsetProfit += S[i].getProfit();
                    }

                    // Evaluate the subset's cost and profit
                    if(subsetPrice <= maxToSpend){
                            if(subsetProfit > maxProfit){
                                    maxProfit = subsetProfit;
                                    if(M != nullptr)
                                            delete[] M;
                                    M = S;
                                    S = nullptr;
                                    mSize = subsetSize;
                            }
                    }
                    else{
                            if(S != nullptr){
                                    delete[] S;
                                    S = nullptr;
                            }
                    }
                    // output code for M
                    if(M != nullptr)
                        delete[] M;

Answer 1

You leak S when (subsetPrice <= maxToSpend) == true but (subsetProfit > maxProfit) == false.

Answer 2

Let's look at what you are doing step by step:

1. Allocate memory for S. Set M to point to null.

   Card * S = new Card[subsetSize];
   Card * M = nullptr;
   

2. If condition A (subsetPrice <= maxToSpend) is met, and condition B (subsetProfit > maxProfit) is met, swap M to point the memory allocated for S, and set S to point to null.

   if (subsetPrice <= maxToSpend){
       if (subsetProfit > maxProfit){
           maxProfit = subsetProfit;
           if (M != nullptr)
               delete[] M;
           M = S;
           S = nullptr;
           mSize = subsetSize;
       }
   }
   

3. If condition A was not met, deallocate the memory S is pointing to.

   else{
       if(S != nullptr){
           delete[] S;
           S = nullptr;
       }
   }
   

4. Deallocate the memory M is pointing to.

   if(M != nullptr)
       delete[] M;
   

So if condition A is met, but condition B is not, then S is neither deallocated or transferred over to M! Memory leak.

Answer 3

Rather than playing detective and tracking down this particular memory leak, I'd advise learning to write your code so you don't cause such problems to start with. The most obvious first point would be to use std::vector instead of trying to handle all the memory management on your own. There is probably no other single step that can eliminate as many problem as quickly as getting used to doing this.

When you use it, almost all problems of this entire class simply cease to exist, because you have an object that owns the memory, and when that object goes out of scope, it releases the memory it owns--entirely automatically. It also works even when/if exceptions are thrown, which your code doesn't even attempt to deal with.

std::vector<Card> subset(subsetSize);

for (int i=0; i<subsetSize; i++) {
    subset.push_back(problemCards[cardIndexesToAdd[i]]);
    subsetPrice += subset.back().getPrice();
    subsetProfit += subset.back().getProfit();
}

if (subsetProfit > maxProfit && subsetPrice < maxPrice) { 
    maxSubset = std::move(subset);
    maxProfit = subsetProfit;
}

// code to print out maxSubset goes here

If you wanted to go even further, you could use (for example) a Boost indirect_iterator in place of your cardIndexesToAdd. This would let you apply standard algorithms directly to the subset you care about. With this, you could pretty easily avoid making a copy of the current subset at all--you'd just use the indirect_iterator to iterate over the original collection in-place instead.

You could also define an operator+ for Card that would sum the Price and Profit fields:

Card operator+(Card const &left, Card const &right) { 
    return Card(left.price+right.price, left.profit+right.profit);
}

With this, and the aforementioned indirect_iterator, adding up the profit for a subset could be something like:

Card subset_stats = std::accumulate(subset.begin(), subset.end(), Card());

Likewise, we could define a comparison operator for Card that produces results based on profit and/or cost:

// Assuming we care primarily about maximizing profit, secondarily about
// price, so if one subset produces more profit, it's better. If they produce
// the same profit, the lower cost wins.
bool operator<(Card const &a, Card const &b) { 
    if (a.profit == b.profit)
       return a.price < b.price;
    return b.profit < a.profit;
}

With that, we can compare Cards directly, like: if (a < b) .. and get meaningful results.

Answer 4

Sorry, this would be a comment but I'm new here and can't do that yet.

No check for nullptr is needed with new for out of memory. Thx@jerry-coffin

All of your delete [] are inside if () or nested if () statements. If this leaks, you missed adding an else with a delete [] and you have missing else statements.

This appears to be a snippet but as it is, I see no reason for M or its assignment of S. You should probably consider having a single delete at the end.