why does this code return -nan(ind)?c++

Question

I'm trying to write a Scaler class that takes a child Test which has a Score.

The Scaler uses it's child's score class's scale method to rescale its value (ex. 1/1 with scale 5 = 5/5 or 0/1 with scale 5 = 0/5).

However whenever I run my code using the DummyTest class as a child it returns a "-nan(ind)/5 instead of 5/5 and my test case fails

TEST_CASE("Scaling test returning 1/1 to 5/5")
{
    auto test = std::make_shared<DummyTest>(Score(1, 1)); // Tests that always returns 1/1
    Scaler scaler(test, 5);

    CHECK(scaler.run() == Score(5, 5));
}

this is my code:

class Score {
public:
    double value;
    double maximum;

    Score() : value(0), maximum(0) { }                          
    Score(double maximum) : value(0), maximum(maximum) { }      
    Score(double value, double maximum) : value(value), maximum(maximum) { }

    Score scale(double scale) const;
    bool success();
};

Score Score::scale(double scale) const {
    double a = (value / maximum) * scale;
    double b = scale;
    return Score(a , b);
}

class Test {
public:
    virtual Score run() const;
};

class DummyTest : public Test {
    Score score;

public:
    DummyTest(const Score& score) : score(score) { }

    Score run() const override {
        return score;
    }
};

class Scaler : public Test {
public:
    Test child;
    double maximum;

    Scaler(std::shared_ptr<Test> child, double maximum) : child(*child), maximum(maximum) { }

    Score run() const override;

};

Score Scaler::run() const {
    Score c = child.run().scale(maximum);
    return c;
}

There's absolutely no need for smart pointers, let Scaler accept a const reference, and you can simply have `DummyTest test(Score 1,1)); Scaler(test, 5);` In general, you shouldn't use smart pointers for function parameters unless in some special cases - have a look at Herb Sutter's [GotW 91](https://herbsutter.com/2013/05/30/gotw-91-smart-pointer-parameters/) for details... — Aconcagua, Aug 25 '18 at 10:14
You don't provide a virtual destructor for your `Test` base class - you won't be able to delete derived `Test` objects via pointer to `Test` (`Test* t = new DerivedTest();`) this way. While you don't do it *yet*, make it a good habit to *always* provide virtual destructors if you have virtual functions. — Aconcagua, Aug 25 '18 at 10:20
What does does CHECK macro do? and you don't have == operator defined here... — Swift - Friday Pie, Aug 25 '18 at 10:20
Using a const reference instead of a smart pointer seems to fix the problem, thank you Aconcagua! — J. S, Aug 25 '18 at 10:54
@J.S But only if you make the `child` member a reference as well, otherwise you get object slicing again, see my answer... — Aconcagua, Aug 25 '18 at 10:58

score 3 · Answer 1 · edited Aug 25 '18 at 16:29

You have gotten a victim to so-called object slicing; I simplified the code a little for better illustration (it does not matter if you receive the pointer via a smart pointer or a raw pointer...)

class Scaler
{
    Test child;
    Scaler(Test* child, double maximum) : child(*child) { }
                                        //   here!!!
};

What happens exactly is that you assign the derived class to an instance of the base class. The base class (as value) cannot hold an instance of the derived class, so all data belonging to the derived class is "cut off", or in other words, only the base class part of the derived class is copied into the tests member. As it now is a true Test instance, it will call Test::run(), which just returns Score(), and you end up in a division by 0...

So now, as you already introduced smart pointers, then profit from:

class Scaler
{
    std::shared_ptr<Test> child;
    Scaler(std::shared_ptr<Test> const& child, double maximum)
           // you don't need the reference counting stuff for the parameter,
           // so you can pass as reference
        : child(child) // assigning the reference to our member
                       // (now with reference count management)
        { }
};

The simpler variant is using raw references:

class Scaler
{
    Test& child;
    //  ^ (!)
    Scaler(Test& child, double maximum)
        : child(child), maximum(maximum)
    { }
};

This even allows simpler code in your test functions:

DummyTest test(Score 1,1));
Scaler s(test, 5);

Be aware, though, that with raw references, you need to make sure that the referenced test lives at least as long as the referencing one (or at least, as long as the referencing test is still using its reference), otherwise you suffer from undefined behaviour. This is guaranteed by the snippet above, not, though, if you do it like this:

 Scaler s(DummyTest(Score 1,1));
 s.run();

Now, after returning from the contructor, the DummyTest instance is destoyed again and you have a dangling reference in s.run().

why does this code return -nan(ind)?c++

1 Answers1