Correct way to use std smart pointers to ensure ptr safety

Question

Is this the correct way to use std smart pointers to ensure ptr safety

This example may not be the best, but I am trying to emulate some real code. The problem I had was in the real code, the communicator pointer was a raw pointer which could be de-allocated at any moment - resulting in a crash in using the pointer.

So I decided to look into std::shared_ptr and std::weak_ptr to see how it should be designed now we have C++11. I use a weak_ptr in the sending code which checks if the ptr is still valid and only then will dereference the ptr. Is this code the correct approach? Any improvements?

#include <memory>
#include <iostream>
#include <string>

class communicator
{
public:
    communicator(const char* name, int comport, int speed) : name_(name), comport_(comport), speed_(speed) { }

    void send(const std::string& s) {
        std::cout << "sending " << s << " using " << name_ << " at " << speed_ << " rate and using com port " << comport_ << '\n';
    }

private:
    const char* name_;
    int comport_;
    int speed_;
};

class sender
{
public:
    sender() {}

    void set_communicator(std::weak_ptr<communicator> comms) {
        comms_ = comms;
    }

    void send(const std::string& s)
    {
        if (auto sh = comms_.lock())
            sh->send(s);
        else
            std::cout << "Attempting to send: " << s << " but ptr no longer exists\n";
    }

private:
    std::weak_ptr<communicator> comms_;
};

int main() {

    sender mysender;

    {
        // create comms object
        std::shared_ptr<communicator> comms(new communicator("myname", 3, 9600));

        mysender.set_communicator(comms);

        mysender.send("Hi guys!");

    }  // comms object gets deleted here

    mysender.send("Hi guys after ptr delete!");
}

Output:

sending Hi guys! using myname at 9600 rate and using com port 3
Attempting to send: Hi guys after ptr delete! but ptr no longer exists

Answer 1

pointer which could be de-allocated at any moment - resulting in a crash in using the pointer

That's the symptom behind the rationale for a introducing a weak_ptr; thus I'd consider your weak_ptr - based approach right.

What I find debatable, however, is that in conjunction with this

sender() : comms_() {}

void set_communicator(std::weak_ptr<communicator> comms) {
    comms_ = comms;
}

sort-of two-phase construction of the the sender 's internal asset comms_ you don't reset the internal asset's state to after-construction-state once lock() fails in

void send(const std::string& s)

But that's not "wrong" by itself; it's just something that could be considered for the full scale app.

The other thing is that you don't throw (or let throw by the shared_ptr(weak_ptr) ctor (#11)) when lock() fails, but just if-else handle that. I cannot know the requirements of your full-scaled app, but based on the extract you assembled, exception based error handling would improve the design imo.

E.g.:

#include <memory>
#include <stdexcept>
#include <iostream>
#include <string>

class communicator
{
public:
    communicator(const char* name, int comport, int speed) 
        : name_(name), comport_(comport), speed_(speed) { }

    void send(const std::string& s) {
        std::cout << "sending " << s << " using " << name_ << " at " 
                  << speed_ << " rate and using com port " << comport_ 
                  << '\n';
    }

private:
    const char* name_;
    int comport_;
    int speed_;
};

class sender
{
public:
    struct invalid_communicator : public std::runtime_error {
        invalid_communicator(const std::string& s) :
            std::runtime_error(
                std::string("Attempting to send: \"") + s 
                    + "\" but communicator is invalid or not set"
            ) {}
    };  

    sender() : comms_() {}

    void set_communicator(std::weak_ptr<communicator> comms) {
        comms_ = comms;
    }

    /* non-const */
    void send(const std::string& s) throw (invalid_communicator)
    {
        try {
            auto sh = std::shared_ptr<communicator>(comms_);
            sh->send(s);
        } catch (const std::bad_weak_ptr& e) {
            comms_ = decltype(comms_)();
            throw invalid_communicator(s);
        }
    }

private:
    std::weak_ptr<communicator> comms_;
};

int main() {
    int rv = -1;
    sender mysender;

    for (auto com : {1, 2, 3}) {
        try {
            { 
                // create comms object
                auto comms = std::make_shared<communicator>(
                    "myname", com, 9600
                );
                mysender.set_communicator(comms);
                mysender.send("Hi guys!");
            }// comms object gets deleted here

            mysender.send("Hi guys after ptr delete!"); 

            // never reached in this example; just to illustrate
            // how the story could continue  
            rv = EXIT_SUCCESS;            
            break; // it'd be not nice to "break", but I did not want to
                   // introduce another state variable
        } catch (const sender::invalid_communicator& e) {
            std::cerr << e.what() << std::endl;
        }
    }

    return rv;
}

live a Coliru's