Sign in to follow this  

C++ smart pointer usage

This topic is 1277 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

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 :)

Share this post


Link to post
Share on other sites

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.

Share this post


Link to post
Share on other sites

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.

Share this post


Link to post
Share on other sites
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. Edited by Álvaro

Share this post


Link to post
Share on other sites

Oh, the code has plenty of stinks tongue.png

 

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 smile.png

Edited by Misantes

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites

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 smile.png

Edited by Misantes

Share this post


Link to post
Share on other sites

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.

Share this post


Link to post
Share on other sites

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?

Edited by Misantes

Share this post


Link to post
Share on other sites

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?


Unfortunately I am unfamiliar with Linux development tools. On Windows I typically use XPerf and the Windows Performance Toolkit from Microsoft, which is a free download on MSDN.

Share this post


Link to post
Share on other sites

Valgrind can be used for this (the callgrind and cachegrind tools in particular) (http://c.learncodethehardway.org/book/ex41.html)

 

However for Linux (and Windows) I really recommend Intel VTune. It is quite pricy but it is trivial to detect hotspots in your code.

 

If you want a cheap hacky alternative and you are using libraries that Emscripten C++ supports, I have recently found that compiling the game to Javascript and then running it in a browser allows me to profile it using Chrome's Javascript profiler which is strangely effective (It almost does a good job at locating out of bounds stack array access, something that ElectricFence, Otto's malloc hack and even Valgrind have difficulty with).

Edited by Karsten_

Share this post


Link to post
Share on other sites

Thanks!

 

And, this question goes out to anyone who can answer it. I'm having trouble getting some of the newer c++ features to be recognized by Code Blocks. When using "make_unique" for example, I get an error saying it's not a member of the std library.

 

I've upgraded my gcc to 4.9, checked via gcc -v that's it's the used version, and tried various compiler settings such as -std=c++11, -std=c++1y,-std=c++14, -std=c++0x, etc. but I still can't get it to recognize make_unique (though, unique_ptr works fine). It's making it a little difficult to test some of these suggestions >.<

 

In theory gcc 4.9 should support it:

http://gcc.gnu.org/gcc-4.9/changes.html

 

Anyhow, thanks again all smile.png

Edited by Misantes

Share this post


Link to post
Share on other sites
Just tested it with -std=c++11 and it is working.
Did you mark it for the entire project?
Have you #included <memory>?

I get an error saying it's not part of the std library.

It would help if you posted the error itself.

Share this post


Link to post
Share on other sites

Thanks for the reply smile.png

Yeah, it's marked for the entire project, and I've included <memory>, and again, the unique_ptrs are working. That's weird it's working for you(or it's weird that it's not working for me). Out of curiosity, are you using gcc? From what I understand, support for make_unique was just recently included in the latest release of gcc 4.9 (it took a little effort to even find a way to upgrade). I'm having a little difficulty finding too much information on using it with codeblocks as it's all relatively new.

the exact error is:

"error: 'make_unique' is not a member of 'std'"

I should also reiterate that I'm on linux, which may change how we're both compiling things.

Edited by Misantes

Share this post


Link to post
Share on other sites
I am also on linux and using GCC 4.9.
Are you using the automatic build system?
What about those "using namespace insertItHere;"?
Go through Project -> Build Options...
and check if you do have the flag marked as in here:
7Z1Dj3E.png

You can also check this by looking at your compiler output when you build your source. Something like this:
g++ -Weffc++ -std=c++11 -Wall -pg -g -DDEBUG -ITextureVault -Iinclude -c /home/dejaime/(...)
Edited by dejaime

Share this post


Link to post
Share on other sites

Yeah, it's the darndest thing. Mine seems to be set up exactly the same and the compiler outputs -std=c++11 as well. I'm making sure I'm setting to the whole program, not just release and debug (they're all ticked to -std=c++11).

I don't use any "using namespace" command, typically, and certainly not right now.

 

I'm a little stumped. I'm wondering if I'm installed correctly, but the "gcc -v" command in a terminal return gcc 4.9.1. I removed 4.8 through synaptic. Maybe I'll try reinstalling 4.9 and see if I have any luck.

 

I'm using code::blocks 13.12, if that makes a difference. I could try using one of the nightly builds, but I've been avoiding them since I haven't had any issues up until this point.

 

I'm definitely still open to suggestions though, if you have any other ideas smile.png

 

My build log looks like:

g++ -std=c++11 -Wextra -fexceptions -O2 -Wall -Iinclude -c /home/****/Documents/Copy/CodeBlocks/OPENGL/Main.cpp -o obj/Release/Main.o 

In case you're able to parse this better than I, here's the full output of gcc -v:

And, apologies for the terrible screenshot below, just open in another window. It shows the "enable common compiler warnings" currently, but disabling that doesn't seem to fix the issue.

 

Using built-in specs.

COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/4.9/lto-wrapper
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Ubuntu 4.9.1-1ubuntu2~14.04.3' --with-
 
+ --prefix=/usr --program-suffix=-4.9 --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --
without-included-gettext --enable-threads=posix --with-gxx-include-dir=/usr/include/c++/4.9 --libdir=/
usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-
time=yes --enable-gnu-unique-object --disable-libmudflap --disable-vtable-verify --enable-plugin --
with-system-zlib --disable-browser-plugin --enable-java-awt=gtk --enable-gtk-cairo --with-java-home=/
usr/lib/jvm/java-1.5.0-gcj-4.9-amd64/jre --enable-java-home --with-jvm-root-dir=/usr/lib/jvm/
java-1.5.0-gcj-4.9-amd64 --with-jvm-jar-dir=/usr/lib/jvm-exports/java-1.5.0-gcj-4.9-amd64 --with-arch-
directory=amd64 --with-ecj-jar=/usr/share/java/eclipse-ecj.jar --enable-objc-gc --enable-multiarch --
disable-werror --with-arch-32=i686 --with-abi=m64 --with-multilib-list=m32,m64,mx32 --enable-multilib 
--with-tune=generic --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --
target=x86_64-linux-gnu
Thread model: posix
gcc version 4.9.1 (Ubuntu 4.9.1-1ubuntu2~14.04.3) 
 
Edited by Misantes

Share this post


Link to post
Share on other sites

Well, that is weird. But you said you had 4.8 before and installed 4.9 manually?

You could check to see if codeblocks is really using the 4.9 version of g++.
There are two ways of doing that.
One would be to compile the code manually via console by copying the g++ command codeblocks uses.
A second possibility is to go through Settings-&gt;Compiler-&gt;Toolchain Executables(TAB) and click in the [...] button by the g++ command. That'll show you the folder where c::b is calling g++ from, you then go to that folder and call g++ --version in it...
&nbsp;
...or C++11 does't have make_unique. I have to admit I haven't really used unique_ptr from the std. Will have to look that up for you.

@edit
Yes, C++11 really doesn't have make_unique. I should have known that! haha
http://en.cppreference.com/w/cpp/memory/unique_ptr (lists make_unique as a C++14 feature)
http://isocpp.org/blog/2013/04/n3656-make-unique-revision-1 (paper that got approved into the C++14 draft)

 

Apparently, my make_unique comes from boost, when I kill the link to boost I get the error. I used to merge boost with my std namespace to mimic C++11 years ago, and this is still biting me to this day every time I reuse that code.

Edited by dejaime

Share this post


Link to post
Share on other sites

Huh, I tried that earlier and it didn't work. I just tried again and it does tongue.png Go figure smile.png

 

Perhaps I wasn't doing it program-wide or something.

 

But, it's working now! Thanks a ton for your help and patience smile.png

(for anyone's reference who sees this later, -std=c++1y, is the one that worked for me)

Edit*

so, on the advice of everyone here, I got both a version working where I don't use pointers at all, and a version where a vector of unique pointers is used, where the vector is filled by emplace_back with make_unique, and the functions just take a reference of the vector.

 

Performance seems like a wash between the two, though I'm getting some weird bugs when not using the pointers. Like, I can't move forward at all, unless I go up a ways in the game first, and some weird things with the UI. I'll try to parse those out, as Alvaro's method seems like it makes the most sense, and unless I can find a good reason why I ought to be using pointers for those, it appears he may be correct. Figuring out what is causing those bugs will likely determine which version I use, but I truly appreciate everyone's help here smile.png

 

Here's the pointer version.

Here's the non-pointer version.

 

Ignore all the other problems. I fixed them once, but then when implementing this stuff, I reverted to an older copy, and just haven't fixed the hard-coding, mixed use casing, etc, etc tongue.png

 

Edit*

This is a little off-topic, but semi related. A problem with my version that doesn't use pointers, is the first 4095 objects in my vector all seem to be at location 0,0,0 in world coordinates (they don't show in game, but report their location there). If I shrink the vector size enough, then the first 2047, or 1023, or 511, etc, etc all show their location there. Any object after that in the vector show up fine. That number makes me feel like it's some sort of size or memory limit or something along those lines for the vector. Or, I'm doing something wrong in creating them with emplace_back. The same problem doesn't exist with the pointer version, and they're largely the same code, minus the pointers.

A quick read over emplace_back shows that it reallocates memory if it goes over the max size. But, I show a capacity of 2048 or whathaveyou, and a huuuuge max_size that I'm absolutely not hitting.

 

But, If anyone has an insight to that, I'd be open to hearing it smile.png

 

Edit*

After some digging, it looks like the vector gets resized as I add elements. I imagine that is where the problem may be stemming from. Calling "vector.reserve(amountToReserve)" beforehand seems to fix the problem. But, I'd still like to know what's really going on. As, eventually, I'm going to want to create a dynamic vector where I don't know the size beforehand :P

Edited by Misantes

Share this post


Link to post
Share on other sites
I didn't look at your code, but it sounds like your objects don't have the right semantics to be copied or moved around.

EDIT: This looks very suspicious:
Object::Object(const Object& orig) {
}
How is that copy constructor supposed to make a copy of anything? Edited by Álvaro

Share this post


Link to post
Share on other sites

Removing that does indeed fix the issue. Now, for the life of me I'm trying to remember why I included that to begin with. I think when I first started learning OpenGL I was still using netbeans, and I built up a little template to open a window a render a couple objects. later, I moved to code::blocks, and I think I just copied the template over. But, I have a feeling netbeans includes that automatically when making a new class. I generally delete it, but for whatever reason had left it in, and just got so used to it being there it went unnoticed until now >.< How embarrassing.

 

But, thanks yet again Alvaro, you've truly been helpful. :)

Edited by Misantes

Share this post


Link to post
Share on other sites
Sign in to follow this