Destructor is called when filling object through assignement

[vindex10]

Bug Report

• What is the issue you have?
  Destructor being called in an unexpected place.

• Please describe the steps to reproduce the issue. Can you provide a small but working code example?

#include <iostream>
#include "nlohmann/json.hpp"

using namespace std;
using json = nlohmann::json;

class State {
public:
    double x;
    ~State() {
        cout << "destroyed" << endl;
    }
};

void from_json(const json& j, State& p) {
    try {
        p.x = j.at("x").get<double>();
    } catch(json::type_error& e) {
        std::cerr << e.what() << std::endl;
    } catch(json::out_of_range& e) {
        std::cerr << e.what() << std::endl;
    }
}

int main() {
    json data;
    State s;
    data["x"] = 10.;
    s = data;
    cout << s.x << endl;
    return 0;
}

Output is:

destroyed
10
destroyed

• What is the expected behavior?
  do not call destructor during assignment

• And what is the actual behavior instead?
  destructor is called

• Which compiler and operating system are you using? Is it a supported compiler?

g++ (Gentoo 6.4.0-r1 p1.3) 6.4.0
Copyright (C) 2017 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

• Did you use a released version of the library or the version from the develop branch?
  checked for both: master and develop branch.

Could you please describe, whether it is a feature or a bug :) By default assignment operator doesn't call destructor, that is why the behavior seems to me unexpected.

Thank you!

[nlohmann]

You see two destructor calls, because there are two objects created:

#include <iostream>
#include "json.hpp"

using json = nlohmann::json;

class State {
public:
    State() : x(0.0) {
        std::cout << "default constructed" << std::endl;
    }
    
    State(const State &o) : x(o.x) {
        std::cout << "copy constructed" << std::endl;
    }
    
    State(State &&o) : x(o.x) {
        std::cout << "move constructed" << std::endl;
    }
    
    State& operator=(const State &o)
    {
        std::cout << "copy assign" << std::endl;
        x = o.x;
        return *this;
    }

    State& operator=(State &&o)
    {
        std::cout << "move assign" << std::endl;
        x = o.x;
        return *this;
    }

    ~State() {
        std::cout << "destroyed" << std::endl;
    }

    double x;
};

void from_json(const json& j, State& p) {
    try {
        p.x = j.at("x").get<double>();
    } catch(json::type_error& e) {
        std::cout << e.what() << std::endl;
    } catch(json::out_of_range& e) {
        std::cout << e.what() << std::endl;
    }
}

int main() {
    json data;
    State s;
    data["x"] = 10.;
    s = data;
    std::cout << s.x << std::endl;
    return 0;
}

Output:

default constructed
default constructed
move assign
destroyed
10
destroyed

The first object is created with State s;, the second when you assign s = data;. Then, a temporary State object is created from data which is then move assigned to s.

The following code avoids this:

int main() {
    json data;
    data["x"] = 10.;
    State s = data;
    std::cout << s.x << std::endl;
    return 0;
}

[vindex10]

Thank you very much for immediate response!

Actually, I had a bit more complicated situation. I'm freeing memory for some properties in the destructor. So the way out is to implement own move-assignment operator, hence move-constructor as well. Is it right workaround?

#include <iostream>
#include <string>
#include "nlohmann/json.hpp"

using namespace std;
using json = nlohmann::json;

class State {
public:
    string* x;

    State() {
        x = new string("hello");
    }

    State(State &&from) {
        x = from.x;
        from.x = nullptr; // delete does nothing with nullptr starting from c++14
    }

    State& operator=(State &&from) {
        if (this == &from) {
            return *this;
        }
        x = from.x;
        from.x = nullptr;
        return *this;
    }

    ~State() {
        cout << "destroyed" << endl;
        delete x;
    }
};

void from_json(const json& j, State& p) {
    try {
        *p.x = j.at("x").get<string>();
    } catch(json::type_error& e) {
        std::cerr << e.what() << std::endl;
    } catch(json::out_of_range& e) {
        std::cerr << e.what() << std::endl;
    }
}

int main() {
    json data;
    State s;
    data["x"] = "world";
    s = data;
    cout << *s.x << endl;
    return 0;
}

Now it works. And output is:

destroyed
world
destroyed

Thank you again!

[nlohmann]

I think so. I'm not sure whether you'd need to call std::move or std::swap at some place - but basically this should be it. To be sure, I'd mark the copy constructor and the copy assignment as delete.

[vindex10]

Yes. Sounds reasonable. Thank you for review :) I like the library very much!