1

This is the scenario: I have a class named Program, which holds three shared_ptr: to the vertex, geometry and fragment shader. When a Shader object gets constructed, it creates the shader with glCreateShader, and it also compiles it.

The Shader denstructor automatically calls glDeleteShader. So the problem is that if I do the following:

  1. Create a shader object;
  2. Copy it;
  3. Destroy the copy.

Also the original copy gets invalidated, because when the copy is destroyed it calls glDeleteShader. That's a design problem, I believe.

So I avoided this problem by just using pointers. Now the Program class holds the shaders. I created a method that returns the shared_ptr to the vertex, geometry and fragment Shader objects. My doubt is: should I return the shared_ptr like this: 

const shared_ptr<Shader>& getVertex_shader_ptr() const
{
    return vertex_shader_ptr;
}

Or like this: 

shared_ptr<Shader> getVertex_shader_ptr() const
{
    return vertex_shader_ptr;
}

My fear is that the problem that I described above occurs again: the shared_ptr gets deallocated and it invalidates the OpenGL shader.

Ramy Al Zuhouri
  • 21,580
  • 26
  • 105
  • 187

4 Answers4

5

Unless it's valid for your shader to be NULL, you should just return a reference to it:

const Shader& getVertex_shader() const
{
    return vertex_shader;
}

If it's valid for your shader to be NULL, but only your program is responsible for deleting it, just return a pointer:

const Shader* getVertex_shader_ptr() const ...

The semantics of a shared pointer is for when there's no clear ownership of the shader. If that's the case, then return by value. The semantics are that two pieces of the program have an interest in the object not being cleaned up, and they both need a way to keep it alive.

shared_ptr<Shader> getVertex_shader_ptr() const ...
Bill
  • 14,257
  • 4
  • 43
  • 55
  • 1
    On "no clear ownership" - that's not the purpose of a `shared_ptr`, but unfortunately it does often seem to be when they're used! I always make sure ownership of objects is clear, and use shared/weak pointers to manage/monitor their lifetime. Perhaps in this case the shader needs to stay alive until it's finished being used, which may be after the owner of the program has been destroyed? We just don't know :) – Ben Hymers Jan 30 '13 at 15:47
  • +1 It doesn't seem like the OP needs a `shared_ptr` at all, all he needs is a copy constructor and assignment operator. – Praetorian Jan 30 '13 at 15:47
  • It's the second case: the shader may be null, but just the program may delete it. – Ramy Al Zuhouri Jan 30 '13 at 15:52
  • 1
    @Bill the whole point of `shared_ptr` implies a return by value. If return by reference works, there's no point in using `shared_ptr`; just return a reference to the actual object. – James Kanze Jan 30 '13 at 16:08
  • @JamesKanze Not necessarily; in fact the recommended way to pass a `shared_ptr` as a parameter is to pass is as a const reference. Or at least it was until move semantics arrived. You're not forcing the caller to store it as a reference, they can keep a copy if they intend to keep it around for a while. – Ben Hymers Jan 30 '13 at 16:32
  • 1
    @RamyAlZuhouri ok, in this case then you don't really need shared_ptrs, unless you need some way to verify that the returned shader is still valid after some time, in which case you probably want to store a `weak_ptr` to it. If not though, you want copy/move constructors/assigment operators, and to return by const raw pointer. – Ben Hymers Jan 30 '13 at 16:34
  • @BenHymers As a parameter, I agree. But I said _return_ by value. – James Kanze Jan 30 '13 at 17:14
  • I need to transfer the ownership of the pointer to the program, but only the program may delete the object. I come from Objective-C, it would be much easier with strong/weak references. – Ramy Al Zuhouri Jan 30 '13 at 17:18
  • @RamyAlZuhouri C++ supports strong and weak references as well, but if you're really transfering ownership, `auto_ptr` (or if you're sure that you can count on C++11, `unique_ptr`) sounds more appropriate. – James Kanze Jan 30 '13 at 17:38
  • @BenHymers - I don't understand what you mean. How can a class which is responsible for deleting the object do so if it has been handing out shared pointers to it? – Bill Jan 30 '13 at 18:57
  • @JamesKanze You did say return by reference, yes, my mistake. It sounded like you were implying there's no point in ever having a reference to a `shared_ptr` which isn't the case. I'd recommend against using `auto_ptr` even if C++11 isn't available; it's hard to use correctly and is deprecated in C++11 for a reason :) – Ben Hymers Jan 31 '13 at 09:03
  • @Bill He said that after I wrote my comment :) Yes, if you only ever want to delete something in one place you shouldn't hand out a `shared_ptr` to it. I figured that since the question was about `shared_ptr` that shared ownership was desired, and was just advising that "shared ownership" isn't (or shouldn't be) the same as "unknown ownership". – Ben Hymers Jan 31 '13 at 09:07
  • @BenHymers Despite claims to the contrary, `std::auto_ptr` works quite well for what it was designed for. And you know you will find it, even if you don't have the latest compiler. – James Kanze Jan 31 '13 at 09:33
  • @JamesKanze Indeed. I just said it's hard to use correctly :) Perhaps not for you and I but certainly for someone asking for help with `shared_ptr`. – Ben Hymers Jan 31 '13 at 14:23
  • @BenHymers Yes, if possible use boost::scoped_ptr or std::unique_ptr instead of std::auto_ptr. – Bill Jan 31 '13 at 15:09
2

Either way is fine here.

It's a member so it's safe to return a reference to it, as long as the caller doesn't hold it by reference then destroy the Program.

If you'd returned by non-const reference then the caller could call reset on your pointer which would call glDeleteShader if no other copies of the pointer exist. As it is though, it'll behave as you want - the Shader will only be destroyed when no more references to it exist.

I'd personally return the shared_ptr by value, but that's just personal preference.

EDIT: Assuming you meant you're worried about correctness when you copy a Program (not a Shader), don't worry about that either; the two Programs will have shared_ptrs to the same Shader which won't be destroyed until both Programs are. This may or may not be what you want (you might want to allocate a completely separate Shader on copy) but it's safe.

Ben Hymers
  • 25,586
  • 16
  • 59
  • 84
2

I would prefer to return by value. If you return by reference, you might run into the chance of a dangling reference to the shared_ptr, if at some point the instance is destroyed and some variable still holds a reference to the shared_ptr.

This situation is exactly what the smart pointers were supposed to avoid, but their reference counting only works safely if you avoid return them by copying. This would invalidate the entire point of using a shared_ptr here.

Piotr99
  • 531
  • 3
  • 12
1

Without knowing much about the underlying shared pointer issues, I found this post by Thiago Macieira (presumably a Qt/Trolltech employee) fairly helpful in getting a better idea about whether to return a value or a const&: http://lists.trolltech.com/qt-interest/2007-11/thread00209-0.html

Quoting from that post, here is the core argument for returning a value from a const function:

The reason why it's:

T at(const Key& k) const;

instead of:

const T &at(const Key &k) const;

is because it's like that everywhere in Qt. We do not return refs-to-const anywhere. In order to return a ref-to-const, we require a variable to exist somewhere, which mandates the internal structure of the class. By returning a value, we can do anything we want internally.

I am not certain though, if that is applicable to your issue... (Still learning myself ;) )

hcc23
  • 1,180
  • 11
  • 17
  • Good point: if it's returned by reference, it may be changed so it's not anymore const. – Ramy Al Zuhouri Jan 30 '13 at 16:22
  • 2
    "... which mandates the internal structure of the class" is the important bit. "... because it's like that everywhere in Qt" is a terrible way to start the explanation :) – Ben Hymers Jan 30 '13 at 16:36
  • 1
    @RamyAlZuhouri : Note that the reference returning function returns a `const T &` (or in the notation I personally prefer, a `T const &`), i.e. a reference-to-const; meaning that although it is a reference, whatever processes the returned value cannot change it. – hcc23 Jan 30 '13 at 16:50