6
\$\begingroup\$

I'm pretty new to templates, and admittedly I'm not particularly good at C++, but I've written a little container that does not move allocated objects once they are created, so you can use weak pointers to access elements, and be aware of their existence. I'm sure that much of it can\should be improved, but I lack the experience to know what exactly. Thoughts and critique are welcome.

#include <memory>
#include <vector>
#include <iostream>

class NSC_OOR_ERR : public std::exception
{
    virtual const char* what() const throw()
    {
         return "NSContainer access out of range";
    }
};

class NSC_BA_ERR : public std::exception
{
    virtual const char* what() const throw()
    {
        return "NSContainer out of memory";
    }
};

template<typename A_Type>
class CNSContainer
{
    NSC_OOR_ERR NSC_OOR_m;
    NSC_BA_ERR NSC_BA_m;
    std::vector<std::shared_ptr<A_Type>> pointers_m;
public:
    size_t size()
    {
        return pointers_m.size();
    }

    template <class... Args>
    std::weak_ptr<A_Type> push(Args&&... args)
    {
        try {
            std::shared_ptr<A_Type> ptr(new A_Type(std::forward<Args>(args)...));
        }
        catch (std::bad_alloc& ba) {
            throw NSC_BA_m;
        }
        pointers_m.emplace_back(ptr);
        return ptr;
    }

    void remove(const size_t& key)
    {
        pointers_m.erase(pointers_m.begin() + key);
    }

    std::weak_ptr<A_Type> operator[](const size_t& key)
    {
        try {
            return pointers_m[key];
        }
        catch (const std::out_of_range& oor) {
            throw NSC_OOR_m;
        }
    }
};

int Test()
{
    CNSContainer<int> container;

    //push into container
    container.push(1);
    container.push(2);
    container.push(3);
    container.push(4);

    //get size
    size_t container_size = container.size();

    //iterate
    for (size_t i = 0; i < container_size; ++i)
    {
        std::weak_ptr<int> wptr = container[i];

        if (!wptr.expired())
        {
            auto sptr = wptr.lock();
            int value = *sptr;
            std::cout << "Value is " << value;
        }
    }

    /*take element pointer to do something with it later.
    Maybe in another thread*/
    std::weak_ptr<int> wptr = container[0];

    /*the pointer now points to nothing*/
    container.remove(0);

    /*but we know that, because it's expired*/
    if (!wptr.expired())
    {
        //do stuff
    }

    return 0;
}
\$\endgroup\$
4
\$\begingroup\$

Thanks for a well-presented question! I'll dive straight in:

Your exceptions are a bit weird. Leaving aside the impenetrable (to me) naming, I think that adding your own exception types derived directly from std::exception just to add your own what() is probably a poor investment of time. If you go this way, I certainly recommend that you subclass the provided std::bad_alloc and std::out_of_range, so that existing code that catches those also catches your equivalents.

Having member variables of exception type in your container instances is decidedly unconventional. We generally create exceptions at the point where they are thrown. This may be important in a debugging environment, if exceptions there are able to carry context information (as happens in Python and Java, for example). In production code, the exception construction will be very low overhead compared with actually raising and catching the exception, so there's little to be gained in speed, and something to be lost by adding unnecessary members to every instance.


A quick skim through the rest of the code draws me to this line:

        std::shared_ptr<A_Type> ptr(new A_Type(std::forward<Args>(args)...));

Bare new in modern C++ always triggers my interest - here, it is better to use std::make_shared(). This will give improved memory locality to the object and its smart-pointer control block.


This gives my version of your code as follows:

#include <memory>
#include <vector>

template<typename A_Type>
class CNSContainer
{
    std::vector<std::shared_ptr<A_Type>> pointers_m;
public:
    size_t size()
    {
        return pointers_m.size();
    }

    template <class... Args>
    std::weak_ptr<A_Type> push(Args&&... args)
    {
        // may throw std::bad_alloc - if so, pointers_m is unchanged
        auto ptr = std::make_shared<A_Type>(std::forward<Args>(args)...);
        pointers_m.emplace_back(ptr);
        return ptr;
    }

    void remove(const size_t& key)
    {
        pointers_m.erase(pointers_m.begin() + key);
    }

    // Will throw std::out_of_range if key is invalid
    std::weak_ptr<A_Type> operator[](const size_t& key)
    {
        return pointers_m[key];
    }
};

At this point, there's almost nothing here that's not in the underlying container, and much that's missing (reserve(), iterators, member types, and more). What you end up with is a convenience function for creating a smart-pointer member in a collection:

#include <memory>

template<typename Container, class... Args>
std::weak_ptr<typename Container::value_type> push(Container& c, Args&&... args)
{
    // may throw std::bad_alloc - if so, c is unchanged
    auto p = std::make_shared<typename Container::value_type>(std::forward<Args>(args)...);
    c.emplace_back(p);
    return p;
}

You might be able to further generalize that to not depend on emplace_back as the insertion method and/or use Concepts to constrain the function and give more informative errors if it's misused.

I'm sorry if the above reduces what you've written to something near-trivial, but I hope you've learnt a lot whilst you were doing it (and that you can apply that to your future projects). Certainly, you seem to have your head around forwarding references, which is more than many can say!

\$\endgroup\$
  • \$\begingroup\$ Thanks for your reply. As it was never my intent to present this code as something complicated, I take no offence to your reduction of it. It's all for the greater goal here. I'd like to ask, when you say that naming is impenetrable, do you refer to class names, or everything in general? How would I improve that? \$\endgroup\$ – BMC Feb 9 '17 at 13:40
  • \$\begingroup\$ It's purely subjective, but I find std::bad_alloc flows off the tongue much more easily than NSC_OOR_m. It's important to fit in with the conventions of your environment: you might have to use that style if the rest of your code is similar. But here, the only context is the language and Standard Library, so I tend to prefer naming that fits in with those. Mostly subjective, though, as I said. \$\endgroup\$ – Toby Speight Feb 9 '17 at 13:47
  • \$\begingroup\$ operator[] does not throw std::out_of_range. \$\endgroup\$ – ncalmbeblpaicr0011 Feb 9 '17 at 21:12
  • \$\begingroup\$ Good point, @ncalmbeblpaicr0011 - can I blame the original code for misleading me? \$\endgroup\$ – Toby Speight Feb 9 '17 at 22:27
3
\$\begingroup\$

Poor Naming

NSC_OOR_ERR NSC_OOR_m;
NSC_BA_ERR NSC_BA_m;

These are terrible names. Instead of having NSC as a common prefix, you could create a namespace. I don't see any benefit to having these exceptions though. If you choose to keep them, you could do something like this:

namespace nsc
{
class OutOfRangeError : public std::exception
{
    virtual const char* what() const
    {
         return "NSContainer access out of range";
    }
};

class BadAlloc : public std::exception
{
    virtual const char* what() const
    {
        return "NSContainer out of memory";
    }
};
} // end of namespace nsc

Personally, I would get rid of these exceptions.

You are appending _m to member variables. The more common convention is to prepend m_ to the member variables. This is an "old-school" thing to do. Most modern C++ programmers would probably avoid any type of name decoration.

Code does not compile

template <class... Args>
std::weak_ptr<A_Type> push(Args&&... args)
{
    try {
        std::shared_ptr<A_Type> ptr(new A_Type(std::forward<Args>(args)...));
    }
    catch (std::bad_alloc& ba) {
        throw NSC_BA_m;
    }
    pointers_m.emplace_back(ptr);
    return ptr;
}

This code should not compile at all. This surely isn't the code you tested.

Logical bug

    try {
        return pointers_m[key];
    }
    catch (const std::out_of_range& oor) {
        throw NSC_OOR_m;
    }

The operator[] function does not do bounds checking, so this will never throw an std::out_of_range exception. Check out the std::vector::at function for range checking.

Other

Check out the Rule of 0/3/5. Check out other container creation tips.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the reply. It is curious that you say that the code doesn't compile. Can you tell me what exactly causes your compiler to fail? Are you implying that ptr goes out of scope after try? As for the 0/3/5 rule, isn't the 0 rule satisfied? \$\endgroup\$ – BMC Feb 9 '17 at 15:55
  • 1
    \$\begingroup\$ @BMC Yes, your code fails to compile on VS2015 because ptr goes out of scope after the try-block and because #include <stdexcept> is missing. I believe you are correct about the rule of 0 being satisfied. \$\endgroup\$ – ncalmbeblpaicr0011 Feb 9 '17 at 17:34
  • \$\begingroup\$ throw() is not the best idea, may be it would be better to replace it with noexcept? Also this. \$\endgroup\$ – Incomputable Feb 9 '17 at 19:04
  • \$\begingroup\$ @Incomputable I didn't even see the throw specifiers; thanks for the catch. For simplicity I'll leave off all specifiers. \$\endgroup\$ – ncalmbeblpaicr0011 Feb 9 '17 at 21:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.