Jump to content

  • Log In with Google      Sign In   
  • Create Account

C++ smart pointer usage


Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.

  • You cannot reply to this topic
22 replies to this topic

#1 Misantes   GDNet+   -  Reputation: 889

Like
0Likes
Like

Posted 17 July 2014 - 08:53 PM

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


Beginner here <- please take any opinions with grain of salt :P


Sponsor:

#2 nobodynews   Crossbones+   -  Reputation: 1916

Like
2Likes
Like

Posted 17 July 2014 - 11:29 PM

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.


C++: A Dialog | C++0x Features: Part1 (lambdas, auto, static_assert) , Part 2 (rvalue references) , Part 3 (decltype) | Write Games | Fix Your Timestep!


#3 Misantes   GDNet+   -  Reputation: 889

Like
0Likes
Like

Posted 17 July 2014 - 11:37 PM

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.


Beginner here <- please take any opinions with grain of salt :P


#4 Álvaro   Crossbones+   -  Reputation: 13326

Like
3Likes
Like

Posted 17 July 2014 - 11:44 PM

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, 17 July 2014 - 11:46 PM.


#5 Misantes   GDNet+   -  Reputation: 889

Like
3Likes
Like

Posted 18 July 2014 - 12:15 AM

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, 18 July 2014 - 01:12 AM.

Beginner here <- please take any opinions with grain of salt :P


#6 Álvaro   Crossbones+   -  Reputation: 13326

Like
1Likes
Like

Posted 18 July 2014 - 12:47 AM

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.

#7 Misantes   GDNet+   -  Reputation: 889

Like
0Likes
Like

Posted 18 July 2014 - 12:52 AM

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, 18 July 2014 - 01:10 AM.

Beginner here <- please take any opinions with grain of salt :P


#8 SmkViper   Members   -  Reputation: 793

Like
1Likes
Like

Posted 18 July 2014 - 08:06 AM

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.

#9 Misantes   GDNet+   -  Reputation: 889

Like
0Likes
Like

Posted 18 July 2014 - 11:07 AM

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, 18 July 2014 - 11:20 AM.

Beginner here <- please take any opinions with grain of salt :P


#10 SeanMiddleditch   Members   -  Reputation: 5872

Like
5Likes
Like

Posted 18 July 2014 - 11:20 AM

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.

#11 SmkViper   Members   -  Reputation: 793

Like
0Likes
Like

Posted 18 July 2014 - 05:28 PM

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.

#12 Karsten_   Members   -  Reputation: 1604

Like
1Likes
Like

Posted 18 July 2014 - 06:27 PM

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_, 18 July 2014 - 06:28 PM.

Mutiny - Open-source C++ Unity re-implementation.
Defile of Eden 2 - FreeBSD and OpenBSD binaries of our latest game.


#13 Misantes   GDNet+   -  Reputation: 889

Like
0Likes
Like

Posted 18 July 2014 - 07:05 PM

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, 18 July 2014 - 07:16 PM.

Beginner here <- please take any opinions with grain of salt :P


#14 dejaime   Crossbones+   -  Reputation: 4027

Like
1Likes
Like

Posted 18 July 2014 - 07:10 PM

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.

#15 Misantes   GDNet+   -  Reputation: 889

Like
0Likes
Like

Posted 18 July 2014 - 07:46 PM

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, 18 July 2014 - 08:10 PM.

Beginner here <- please take any opinions with grain of salt :P


#16 dejaime   Crossbones+   -  Reputation: 4027

Like
1Likes
Like

Posted 18 July 2014 - 08:14 PM

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, 18 July 2014 - 08:20 PM.


#17 Misantes   GDNet+   -  Reputation: 889

Like
0Likes
Like

Posted 18 July 2014 - 08:48 PM

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) 
 

Attached Thumbnails

  • tests.png

Edited by Misantes, 18 July 2014 - 09:23 PM.

Beginner here <- please take any opinions with grain of salt :P


#18 dejaime   Crossbones+   -  Reputation: 4027

Like
1Likes
Like

Posted 18 July 2014 - 09:16 PM

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, 18 July 2014 - 09:34 PM.


#19 Misantes   GDNet+   -  Reputation: 889

Like
0Likes
Like

Posted 18 July 2014 - 09:35 PM

ah, doh!

 

I knew that it didn't (only after getting the error. But, that's why I upgraded gcc), but the 4.9 version of gcc was supposed to enable it. Though, must be for c++14 I think. Do you know of a way to get code blocks to use c++ 14?


Beginner here <- please take any opinions with grain of salt :P


#20 dejaime   Crossbones+   -  Reputation: 4027

Like
1Likes
Like

Posted 18 July 2014 - 09:40 PM

It is probably -std=c++14 or -std=c++1y

But you'll have to add it in the [Other Options] tab and unmark the -std=c++11 .






Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.



PARTNERS