Sign in to follow this  

Passing std::unique_ptr as const ref

Recommended Posts

Hi all,

Since a while I've started to work more with std::unique_ptr, which gives me control on when to construct an object etc.

Now in some cases some class needs a const ref to an object which is created as a std::unique_ptr.

 

I've been searching on how this should be handled, from what I understand, it's perfectly fine/ OK to pass the std::unique_ptr as *myPtr, when a const ref is expected.

 

From practice/ playing around I see that this works.

My question; would you see any cons in doing it this way?

 

Any input is appreciated, like always.

Share this post


Link to post
Share on other sites

Hi all,

Since a while I've started to work more with std::unique_ptr, which gives me control on when to construct an object etc.

Now in some cases some class needs a const ref to an object which is created as a std::unique_ptr.

 

I've been searching on how this should be handled, from what I understand, it's perfectly fine/ OK to pass the std::unique_ptr as *myPtr, when a const ref is expected.

 

From practice/ playing around I see that this works.

My question; would you see any cons in doing it this way?

 

Any input is appreciated, like always.

 

If you are dealing with C api or external libraries you must use the const ref variant.

In design perspectives, it's usually better to pass a unique ptr, because this clerifies the usage of the pointer (which is the goal of smart pointers).

I don't recommend passing regular pointers (or const *) when you clearly has a Unique_ptr.

If you really have to share the memory (In terms of memory owners) then use a shared_ptr instead of passing the raw pointer. 

Share this post


Link to post
Share on other sites
What Zipster said, x10000.

Smart pointers are there to describe ownership semantics; passing around const refs to unique_ptrs doesn't make a huge amount of conceptual sense - the thing you are passing to can't own, so all you are doing it making a meal of the function signature for no good reason.

Even shared pointers are a bit of an iffy case; often just thrown in because people aren't sure when generally correct design can resolve the majority of cases.

If you are't taking ownership of an object then don't pass in smart pointers would be my default setting - there is nothing wrong with a raw pointer when the design calls for it.

Share this post


Link to post
Share on other sites

Thanks guys.

Ownership of the pointer isn't an issue/topic here, so I'll just pass the raw ptr of the unique_ptr to functions where a const ref is expected. I don't want to pass an actual unique_ptr in these cases. For example, I have a IDevice class that has a 'SetRenderTarget' function, which takes a const ref to a IRenderTarget. And when the 'calling' code has a std::unique_ptr for the IRendertarget object, I'll pass *myRT to pass the object to the IDevice function (which expect a const ref).

Share this post


Link to post
Share on other sites

Ownership of the pointer isn't an issue/topic here

It is. That's what Zipster was trying to explain. It is perfectly fine to pass a reference to an unique pointer from a technical point of view. It just isn't 100% "correct" in semantics.
A unique pointer not only manages the resource but also communicates that your intent is there shall be exactly one such pointer which owns the resource. You can move it, but not copy. If you move it, you must do it explicitly, and you thereby communicate to the human reader that you wish to transfer ownership.

When you create a reference, you're cheating. You are creating a "copy" without creating one. The reference is merely an alias with a different name, sure, so technically this is perfectly correct. But it looks like something different.

I don't want to pass an actual unique_ptr in these cases.

You cannot even do that even if you wanted, the class is designed to not allow this (that's the major difference to it's predecessor auto_ptr). You can only move the unique pointer, which however turns the function into a sink.
Now you can be especially devious and create a reference to the unique pointer, and then move the unique pointer to your function. That, too, is perfectly "correct" (until you access the reference). Only just, when the function returns, the unique pointer that your reference aliases will have been destroyed. Good luck trying to debug that, it might even accidentially work for a while.

Long story short: References to smart pointers are not the most awesome idea in the world.

Share this post


Link to post
Share on other sites
So I get that passing references to unique_ptr's isn't very good. But what about passing collections (say a vector) of unique_ptrs? If I have objects stored in a vector as smart pointers, wouldn't it be problematic to have to make a new vector with raw pointers just to pass the vector to a function?

Share this post


Link to post
Share on other sites

I've been searching on how this should be handled, from what I understand, it's perfectly fine/ OK to pass the std::unique_ptr as *myPtr, when a const ref is expected.
 

 

As others already have explained, this is fine. Just as a comment, I use myPtr.get() instead of *myPtr for extra clarity.

Share this post


Link to post
Share on other sites

Either I'm confused, or I'm seeing a lot of confused responses.

 

 

There is nothing wrong with passing objects by reference to const at almost any time, regardless of the storage duration of that object (static, automatic, or dynamic held by a std::unique_ptr).  It's in fact an excellent way to structure your code and I'd recommend it above all others since it's clearer, simpler, and idiomatic.

 

Second best would be passing a naked pointer to const, but then for the static and automatic cases you'd have to decorate the calls with pointer-of operators and call the .get() member function on smart pointers, and you'd lose all of the advantages introduced to the language by references in the first place.  Whether to use naked pointers to const or references to const is a design choice most often based on the use of null pointers for in-band signalling of special cases, something that can often be better handled through superior design that follows the SRP.

 

So to the original post, yes, that's the right thing to do.

Share this post


Link to post
Share on other sites
I haven't been keeping up with C++ best practices in the last few years, but it would seem to me that taking a const reference to std::unique_ptr<Foo> is confusing.

I would expect most of the time you just want to pass around a pointer or reference to Foo, and the ownership stays with the caller. There will be some exceptional circumstances where you want to transfer ownership of the pointer to the function, which is when I you pass an std::unique_ptr<Foo>. But what is the intent when you pass a `std::unique_ptr<Foo> const &'? I really don't know.

So to the original post, no, because the intent is not clear.

Share this post


Link to post
Share on other sites
But what is the intent when you pass a `std::unique_ptr const &'? I really don't know.

 

I could see it as a way of stating "This pointer is not supposed to outlife the function, so it should never be stored", but its really not clear at all, and even when used throughout the codebase, would still be ambiguous.

 

I totally agree with the notion, that if you don't care about ownership at a certain point, you just pass plain pointers/references. If you ie. have a composite object, where all leaves have a backpointer to their owner, there is no need to have the leaves know about the actual storage type of their owning composite. The composite will outlive the leaves, so each leave gets a Composite*. The additional benefit is that now composites can be stored however they want - as a unique_ptr, shared_ptr (if some code requires it), or some custom memory managment scheme - the code will still work. Whereas if you force "void setParent(const std::unique_ptr<Composite>& parent)", it would mean that the parent would have to be a unique_ptr all the time. An additional bonus of not using unique_ptr through your interface, if you will.

 

 

 

There will be some exceptional circumstances where you want to transfer ownership of the pointer to the function, which is when I you pass an std::unique_ptr.

 

TBH, I found that quite commonly. Ok, I don't use custom allocators throughout my codebase, but when you just replace every regular occurence of "new" with "std::make_unqiue", you'll often call a function with eigther "std::unique_ptr<> func()" if you want to create a new object, or "void func(std::unique_ptr<>&& ptr)" if you want to set an object. (IDK if std::unique_ptr could or would still be used if you used a complete memory allocation managment scheme, but since I have little experience with it, take my opinion with a grain of salt).

Edited by Juliean

Share this post


Link to post
Share on other sites

I haven't been keeping up with C++ best practices in the last few years, but it would seem to me that taking a const reference to std::unique_ptr<Foo> is confusing.

I would expect most of the time you just want to pass around a pointer or reference to Foo, and the ownership stays with the caller. There will be some exceptional circumstances where you want to transfer ownership of the pointer to the function, which is when I you pass an std::unique_ptr<Foo>. But what is the intent when you pass a `std::unique_ptr<Foo> const &'? I really don't know.

So to the original post, no, because the intent is not clear.

 
See, that's where I'm confused because I see nowhere where @cozzie asked that, only where they asked about passing a *std::unique_ptr<T> as an argument to a T const& parameter.
 
Perhaps cozzie can clarify his question more with a small code example?

Share this post


Link to post
Share on other sites
Just for future reference, when adding .get() in the sample case above, you still have to use * to identify the pointer. So: *myPtr.get()

This is weird, you shouldn't need to do that.

 

The idea is that you wrap an object in a std::unique_ptr<> to express it has a single owner at all times.

One object has that std::uniqe_ptr as object. That object owns the wrapped object. It can do "*" and "->" operations, just asif the std::unique_ptr<> object is a pointer to the wrapped object, ie

class X {
  std::unique_ptr<Y> mine;

  void q() {
      ...
      mine->f();  // Access the 'f' function of the 'Y' object.

For other (non-owning) objects, you give out bare pointers

Y* X::getMine() { return mine.get(); }

this means they use the wrapped object, but do not own it.

 

They do something like

void Z::g(X &x) {
    Y* y_ptr = x.getMine();
    y_ptr->f();
    ....
}

So yeah, you do get a double "*" indirection, but never on the pointer you ".get()" on, as it doesn't live in Z, it lives in X.

Edited by Alberth

Share this post


Link to post
Share on other sites
Just for future reference, when adding .get() in the sample case above, you still have to use * to identify the pointer. So: *myPtr.get()

 

This is not correctly stated. You need to use the unary * operator here to dereference the pointer returned by get() (not "identify," that makes no sense). This is because you're using the expression as an argument to a function call where a reference, not a pointer, is expected. This is not necessary if you are using the result of get() as an argument to a function where a pointer is expected.

Edited by Josh Petrie

Share this post


Link to post
Share on other sites

I haven't been keeping up with C++ best practices in the last few years, but it would seem to me that taking a const reference to std::unique_ptr<Foo> is confusing.

I would expect most of the time you just want to pass around a pointer or reference to Foo, and the ownership stays with the caller. There will be some exceptional circumstances where you want to transfer ownership of the pointer to the function, which is when I you pass an std::unique_ptr<Foo>. But what is the intent when you pass a `std::unique_ptr<Foo> const &'? I really don't know.

So to the original post, no, because the intent is not clear.

 
See, that's where I'm confused because I see nowhere where @cozzie asked that, only where they asked about passing a *std::unique_ptr<T> as an argument to a T const& parameter.
 
Perhaps cozzie can clarify his question more with a small code example?


Upon re-reading the original post, I think you are correct. A few lines of code to clarify would be great.

Share this post


Link to post
Share on other sites

OK, time for some more context and clarification :rolleyes: 

 

- I'm practicing and applying 'abstraction' of my low-level renderer

- one example: I've created a class IRenderTarget (parent), which has an inherited DX11RenderTarget class

- in the low-level renderer there's a member: std::unique_ptr<IRenderTarget> mTempRT;

- because I'm using dx11 as API: I'm creating a DX11RenderTarget:

	mTempRT = std::move(std::make_unique<DX11RenderTarget>(mDevice.get(), mSettings.GetScreenWidth(), mSettings.GetScreenHeight(), mBackBufferFormat, mSettings.GetMSAANrSamples(), mSettings.GetAAQuality(), false));

- now the low-level renderer also has a member std::unique_ptr<IDevice>, which in the same way is created as a DX11 device (inherited class):

	mDevice = std::move(std::make_unique<DX11Device>(pHwnd, &mSettings, mBackBufferFormat, mDepthBufferFormat));

- the IDevice class has a number of virtual functions, inherited in the DX11Device class

- for example, one to set the current RenderTarget:

	bool SetRenderTarget(const IRenderTarget &pRT);

- this member function shouldn't do anything non-const with the rendertarget, so I decided to let the parameter be a const ref

- and to pass my mTempRT to the Device/ this function, I call it like this:

	if(!mDevice->SetRenderTarget(*mTempRT.get())) return false;

I hope this gives some clarification.

Share this post


Link to post
Share on other sites

maybe I'm missing something but why do you wrap the result of make_unique in std::move above? The result of make_unique is already an rvalue, so the std::move is redundant.

 

Back to the topic at hand, I happen to agree with the rules written here by Stroustrup and Sutter: http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-in

F.16: For “in” parameters, pass cheaply-copied types by value and others by reference to const

 

So you're doing it right, and what you are communicating through this function signature (const IRenderTarget &pRT) is:

1) this method has no interest in the ownership of the object being passed, and does not sink or re-seat the pointer, otherwise you would pass (unique_ptr<IRenderTarget>), see rule F.18

2) this is an "in" parameter, otherwise you would have used (IRenderTarget&) or (IRenderTarget*)

3) it cannot be passed no value (aka nullptr), otherwise you would have used (const IRenderTarget* pRT)

 

This is a nitpick and a matter of personal preference but I would not prefix a const ref parameter name with "p".

 

On a slight tangent, I would prefer to sink those objects in to member variables via constructor parameters, dependency injection style, rather than calling "make_unique" within the Renderer's constructor.

Share this post


Link to post
Share on other sites

@y2kaih: the std::move addition is needed because it's an abstract class

 

That's just I choice I made on how to influence the order of construction of my objects.

I could also use normal member variables instead of unique_ptr, and then let them have some init/deinit or startup/shutdown function, but I prefer this method.

 

For illustration:

// renderer class definition

class CRenderer
{
public:
	CRenderer();
	~CRenderer();

	CRenderer(const CRenderer& other) = delete;					// copy constructor: not allowed
	CRenderer(CRenderer&& other) = delete;						// move constructor: not allowed
	CRenderer& operator=(const CRenderer& other) = delete;		// copy assignment: not allowed
	CRenderer& operator=(CRenderer&& other) = delete;			// move assignment: not allowed

	IDevice* GetDevice()		const;

	// API-independent
	bool LoadSettings(const std::string &pFilename);
	bool Startup(HWND pHwnd, const std::string &pRootFolder);

	bool SetFullscreen(const bool pFullscreen);
	bool ChangeAASettings(const bool pEnabled, const uint pNrSamples);

	bool PrepareRender(const float pColor[4]);
	bool Present();

	// DX11 Specific!
	bool OnResize();

	// API-independent
	CRendererSettings				mSettings;

	std::unique_ptr<IRenderTarget>	mTempRT;
	std::unique_ptr<CViewport>		mTempViewport;


// renderer constructor

CRenderer::CRenderer() :
	mRenderTargetMgr(nullptr), mCBufferMgr(nullptr), mShaderMgr(nullptr), mPrimaryRTHandle(0)
{	
	mBackBufferFormat = CR_FORMAT_RGBA8_NORM_SRGB;
	mDepthBufferFormat = CR_FORMAT_DP24_NORM_ST8_UINT;
}

// renderer Startup

bool CRenderer::Startup(HWND pHwnd, const std::string &pRootFolder)
{
	/** Create GPU device **/
	mDevice = std::move(std::make_unique<DX11Device>(pHwnd, &mSettings, mBackBufferFormat, mDepthBufferFormat));
	
	if(!mDevice->Startup())
	{
		CLOG(FATAL, "RENDERER") << fatal_startup_device.c_str();
		return false;
	}


	mTempRT = std::move(std::make_unique<DX11RenderTarget>(mDevice.get(), mSettings.GetScreenWidth(), mSettings.GetScreenHeight(), mBackBufferFormat, mSettings.GetMSAANrSamples(), mSettings.GetAAQuality(), false));
	mTempViewport = std::move(std::make_unique<CViewport>(0, 0, mSettings.GetScreenWidth(), mSettings.GetScreenHeight(), 0.0f, 1.0f, true));

	/** Set viewport and rendertarget **/
	if(!mDevice->SetViewport(*mTempViewport.get())) return false;
	if(!mDevice->SetRenderTarget(*mTempRT.get())) return false;

Edited by cozzie

Share this post


Link to post
Share on other sites

the std::move addition is needed because it's an abstract class

 

That's not true. The move is just superfluous.

 

Interesting... I will look into that, thanks. Can you provide a reference?

 

As above, not true. Note the available operator= overloads for unique_ptr.  As long as U is a child of T, a unique_ptr to U may be on the right-hand side of an assignment to a unique_ptr to T without anything special like a move operation; the move would not, in fact, change anything about the types involves anyhow so it's redundant.

#include <iostream>#include <memory>


struct Abstract {
    virtual void f() = 0;
};


struct Child : Abstract {
    void f() { std::cout << "Hello, world!\n"; }
};


int main()
{
  std::unique_ptr<Abstract> p;
  p = std::make_unique<Child>();
  p->f(); // prints "Hello, world!"
}

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this