Hey all,

Two part question here:

I asked for a code review a couple days ago, and on the advice of some friendly people, it was recommended I not use raw pointers. Smart pointers were entirely new to me, since all the tutorials and books I've used thus far were apparently a couple years outdated. After reading about them for a bit I tried to implement them, and the code is working, but I still wonder if I'm doing things correctly.

Basically, i need a vector of pointers to hundreds of objects. So, I've created a vector of unique_ptrs. This, of course, broke all of my functions that had that vector being passed in. So, I changed all of those functions to return a vector of unique_ptrs. And, instead of passing in the vector, i just set it to equal the returned value.

//something like this
std::vector<std::unique_ptr<Object> > Function1(std::vector<std::unique_ptr<Object> > alteredVector)
{
//do stuff
return alteredVector;
}

int main()
{
   std::vector<std::unique_ptr<Object>> firstVector;
   firstVector = Function1(move(firstVector));
}

This just feels a little inelegant, and since this is my first implementation of them, I worry I'm going about this the wrong way entirely. Would this be a case where I ought to use shared_ptrs instead or is this entirely fine? I think I understand the uses of the unique_ptr, but I'm a little fuzzy on shared_ptrs still.

And, then the second question, is whether I still need to clear the vector in the destructor or whether it's fine since all the pointers are deleted?

In case my example is poor, the actual implementation is here:

https://github.com/Hobogames/GameTest/blob/master/Main.cpp

https://github.com/Hobogames/GameTest/blob/master/Main.h

Thanks in advance!

Cheers :)

I'm not sure what you did that 'broke all of [your] functions that had that vector being passed in', but you should be able to just pass in the std::vector of std::unique_ptrs as a reference. I tested it to be sure (and utilized typedefs; I'd suggest doing the same if you'd like fewer nested brackets):

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

class Planet
{
public:
    int X;
    int Y;
};

typedef std::unique_ptr<Planet> Planet_ptr;
typedef std::vector<Planet_ptr> Planet_vctr;

void Function1(Planet_vctr &alteredVector)
{
    Planet_ptr p(new Planet);

    alteredVector.push_back( move(p) );
}

void Function2(const Planet_vctr &constVector)
{
    if (constVector.size())
    {
        std::cout << "Not Empty!\n";
    }
    else
    {
        std::cout << "Empty!\n";
    }
}


int main() {
    Planet_vctr aVector;
    Function2(aVector);
    Function1(aVector);
    Function2(aVector);
}

Without delving into how you're using the pointers, I wouldn't be able to offer a suggestion on whether to use unique_ptr or shared_ptr. I found this explanation from Stack Overflow fairly coherent that might help you.

Thanks for the quick reply!

Hm, I was under the impression that the pointer would be deleted at the end of the function it is passed in to. A little testing seemed to back that up. If I add a std::cout that counts off the iterations of the function it will run once then segfault the second time.

A little later I'll have time to really look over your example and see where it is I'm going wrong. Maybe I was just failing to pass it in as a reference.

I didn't follow the other thread, but I don't see why you need to use unique_ptr at all. It would make sense if Planet were an abstract class and you had several different implementations, but nothing derives from Planet. So you could just use a vector of Planet and that way you don't need pointers at all.

I don't think `move' means what you think it means. If the point is that you are not going to be using the old value of alteredVector anymore, you should probably make your function's prototype be `void Function1(std::vector<std::unique_ptr<Object>> &alteredVector)'.

Your code has a few other stinks, like this one:

std::vector<std::unique_ptr<Planet>> planetAndStarsVector;

If the type is that of a vector of planets, why does the name contain `AndStars'? That's either wrong (likely) or at the very least requires a comment.

Oh, the code has plenty of stinks

The name "planetAndStarsVector" is just probably poor coding on my part. A more apt name for the class would be "CosmicObject" or some such. The planets and stars all exist in the same vector and then the functions parse them apart based on their type. The Planet class encompasses both (terrible naming to be sure) as originally they were very similar, minus their orbits. You're totally right though, as time goes on, they've become different enough where it's biting me in the **&^ and just making everything complicated as in every function I have to have two sub-functions for each. I'm in the process of rewriting most of the code, so that's definitely something I'll likely separate into two distinct classes.

As to your point about not needing pointers, and at the risk of appearing dumb, is it possible to push back lots of instances of a class into a vector without using "new" (I'm under the impression using "new" requires the use of pointers to reference them, though in hindsight I could be mistaken here)? I need hundreds of them with semi-randomized values. And, if so, how would I go about doing this ( I would love to not use pointers for this). Is everything largely the same, just minus the pointer?

It's my first opengl program though, and I just started programming in January(with so much of that time spent on different engines, languages and such), so that's why I put it up for review, warts and all. I want to fix as many bad programming habits as possible, find any gaping holes in my programming knowledge(no small feat), and clean up the code before I start implementing any actual game mechanics. But, things are a bit of a functional mess now as the program began with me just opening a window, then started adding things and rewriting things without much planning or foresight involved. So, call things as you see 'em, no hard feelings, and I appreciate the input

You don't need `new'. Just use emplace_back to create new objects at the end of the vector. You can do that with as many instances as you want.

ack, now I feel silly, I had no idea that existed. Thank you Alvaro, I think this is like the 3rd or 4th time you've genuinely helped me. I'll read up on that tonight, implement it tomorrow and post how it goes.

And, please, if you notice(d) any other glaring missteps, please feel free to point them out here:

Thanks again

You don't need `new'. Just use emplace_back to create new objects at the end of the vector. You can do that with as many instances as you want.

Keep in mind that this may come with a speed tradeoff if you are modifying your vector, moving things around in it, or extracting values. This is because you are now storing entire objects in the vector, so any time the vector needs to resize itself, or sort, or pull a value out you will be making copies. Even with move constructors/operators you're still going to be moving around more memory then you would if you stored pointers, assuming your class is larger then 32/64 bits in size (depending on the platform you're building on).

To determine if it adversely affects your code, you'll have to measure with a proper profiling tool. It may be that in your case having all the objects adjacent in the vector (thereby potentially reducing cache misses) and avoiding allocation costs for each one may end up being higher performing then using smaller sized pointers and allocations.

Thanks for the heads up. My initial implementation resulted in an fps drop, but it was a half hearted attempt and I hadn't fixed all the functions, so it could have been due to a number of reasons. I'll try out both methods, and see which gives better results.

Do you have a profiling tool you recommend? Profiling is embarrassingly a new concept to me. I'm on linux using code-blocks. Would valgrind be appropriate, or is that only for memory leaks?

typedef std::unique_ptr<Planet> Planet_ptr;
typedef std::vector<Planet_ptr> Planet_vctr;

In C++11, prefer:

using Planet_ptr = std::unique_ptr<Planet>;

This will be more consistent with idiomatic C++ and any case where you need a templated aliases (which typedef cannot do).

Planet_ptr p(new Planet);

Never call operator new unless you're doing something funky. It's handy to be able to search your codebase for "enew" and find all the dangerous you're-probably-leaking points easily. Prefer:

auto p = std::make_unique<Planet>();

Or, for other smart pointers, their corresponding make_whatever<Type>(parameters...) functions.

    alteredVector.push_back( move(p) );

While it won't necessarily have much impact in this specific case, get in the habit of using emplace_back instead of constructing an object and then pushing it, e.g.

alteredVector.emplace_back(std::make_unique<Planet>());

It avoids some unnecessary copies/moves, which can matter for certain types.

Without delving into how you're using the pointers, I wouldn't be able to offer a suggestion on whether to use unique_ptr or shared_ptr. I found this explanation from Stack Overflow fairly coherent that might help you.

The answer is almost never to use shared_ptr. It's handy in some concurrenct algorithms, and otherwise is a viper pit of hard-to-track ownership problems. Have one clearly-defined owner using unique_ptr, and have everyone else use a raw pointer, a reference, a custom handle type.