Recommended way of returning pointer from C++ function?

Question

I have a pointer that is passed to a series of functions, where one function returns an address and assigns that address to the pointer. After seeking some help on Stackoverflow, the collect_file_path method has a parameter of type QStringList**. Although I understand what is going on I have not come across this notation in any of the text books I own, plus, it looks ugly.

I would like some advice/feedback on how other programmers would implement what I have done in the code below. Would you use QStringList**, or some other method?

I hope what I'm asking makes sense. My current code is:

void ThreadWorker::run()
{
  QStringList* file_list;
  collect_file_paths(&file_list);
}

void ThreadWorker::collect_file_paths(QStringList** file_list)
{
  DirectorySearch ds;
  *file_list = ds.get_file_names(_strPath);
}

QStringList* DirectorySearch::get_file_names(QString path)
{
  QStringList *file_names = new QStringList;
  traverse(path, file_names);
  return file_names;
}

Thanks

@rmflow: For the job of returning managed dynamically-allocated resources, I would use `auto_ptr` (`unique_ptr` where available), for the reason that it can be implicitly converted to `shared_ptr` if that's what the caller wants, or they can get the raw pointer out with `release()` if they have some other smart ptr that they prefer to `boost::shared_ptr`. For example, they might prefer `std::shared_ptr` where available, or some other smart pointer that I've never heard of. If you return `shared_ptr` they would then end up needing a `their_ptr >` — Steve Jessop, Jun 21 '11 at 11:23

score 3 · Answer 1 · answered Jun 21 '11 at 09:57

3

The problem with returning a bare pointer is that it's quite easy to end up leaking the memory for the object. This might happen for example if anything in your function throws; but it can also happen on the caller's side, before they have received the pointer (e.g. if it is used as an argument to a function, and the evaluation of another argument throws).

For these reasons, it's best to wrap pointers in a smart pointer like std::auto_ptr (unique_ptr in the upcoming C++ standard) or boost::shared_ptr (also available as std::shared_ptr, soon in a standard library near you). These wrappers will safely take care of deleting the wrapped pointer when the time comes to do so.

However, in this specific case you can manage without using a QStringList** by passing the QStringList* as a reference.

answered Jun 21 '11 at 09:57

Jon

428,835
81
738
806

that's not a problem with pointers, that's a problem with programmers! – James Jun 21 '11 at 10:00
@James: Well, I think it's fair to say that modern C++ style makes it practically impossible to write exception-safe code in the presence of bare pointers. Of course this can be solved by writing more code for smart pointers, but I would certainly describe the whole affair as very difficult to get right even for experienced programmers. – Jon Jun 21 '11 at 10:08
Returning `shared_ptr` is antisocial, since it prevents the caller from using any other smart pointer type to manage the memory (the semantics of shared_ptr being that for all you know, the function that returned it has retained its own reference. There's no clean way to release the pointer from the shared_ptr). Returning single-owner smart pointers is fine, and IMO returning a raw pointer is fine provided that it's obvious the function allocates resources, and hence obvious to the caller those resources need to be stuffed directly into some resource manager of their own choosing. – Steve Jessop Jun 21 '11 at 11:13
Oh, and note that "prevents the caller using any other smart pointer type" means in particular that `boost::shared_ptr` and the new `std::shared_ptr` don't mix, despite being pretty much identical. So if you think you might ever migrate from one to the other, and your APIs enforce one or the other, AFAIK you're looking at a Great Big Simultaneous Change Everywhere. Not sure whether or not `namespace boost { using std::shared_ptr; }` solves this, maybe it does. – Steve Jessop Jun 21 '11 at 11:15

score 2 · Accepted Answer · answered Jun 21 '11 at 09:56

2

You can pass by pointer reference also, which is more C++ style:

void ThreadWorker::collect_file_paths(QStringList*& file_list) <-- see *&
{
}

You don't have to pass the address of file_list now:

collect_file_paths(file_list);  // simply pass it

But instead of that, I would still recommend following approach (which is simpler):

void ThreadWorker::run()
{
  QStringList* file_list = collect_file_paths();
}

QStringList* ThreadWorker::collect_file_paths()
{
  DirectorySearch ds;  //<---- ds uninitialized
  return ds.get_file_names(_strPath);  // for static function use ClassName::method() style
}

answered Jun 21 '11 at 09:56

iammilind

68,093
33
169
336

What do you mean uninitialized? ds is just an instance of the DirectorySearch class. – nf313743 Jun 21 '11 at 11:50
@johnnyturbo3, since `ds` is not modified anywhere till the `return`, I suspect that in calling `get_file_names` there is no contribution of the `DirectorySearch` object. So to make the design proper you should declare `get_file_names` as `static` method inside the `class`. However, this is just a guess; you better know the design. – iammilind Jun 22 '11 at 02:50

score 2 · Answer 3 · answered Jun 21 '11 at 10:01

2

Just return by value. The compiler can RVO, NRVO and you can swaptimize.

answered Jun 21 '11 at 10:01

Puppy

144,682
38
256
465

hansmaad · Answer 4 · 2011-06-21T11:17:20.970

2

You can return by value, since unnecessary copies will be optimized away. This is a clear, exception safe and my recommended way.

void ThreadWorker::run()
{
  QStringList file_list = collect_file_paths();
}

QStringList ThreadWorker::collect_file_paths()
{
  DirectorySearch ds;
  return ds.get_file_names(strPath_);  // you should not use leading underscore in c++
}

QStringList DirectorySearch::get_file_names(QString path)
{
  QStringList file_names;
  traverse(path, &file_names);
  return file_names;
}

http://cpp-next.com/archive/2009/08/want-speed-pass-by-value/

Btw. I think Qt Containers have copy on write optimizations, so the copy will be cheap even without compiler optimizations.

If you don't trust in (very common) compiler optimizations like RVO (comment by heishe) and you do not use copy-on-write objects like the QStringList you should create the instance in the run() method and pass a reference to the other functions. This is not as clear as my recommended technique but it is still exception safe (at least the basic guarantee).

void ThreadWorker::run()
{
  QStringList file_list;
  collect_file_paths(file_list);
}

void ThreadWorker::collect_file_paths(QStringList& file_list)
{
  DirectorySearch ds;
  ds.get_file_names(strPath_, file_list);  
}

void DirectorySearch::get_file_names(QString path, QStringList& file_list)
{
  traverse(path, &file_list);
}

A 3rd solution is to return a smart pointer like std::unique_ptr<QStringList>. But I cannot see any reason for an additional dynamic allocation in this example.

edited Jun 21 '11 at 11:17

answered Jun 21 '11 at 10:01

hansmaad

18,417
9
53
94

I always assumed that pass by reference was much faster than pass by value. – nf313743 Jun 21 '11 at 10:08
They are, and that's why I "-1"'d his answer. He's principally right in what he says, that compilers can optimize away here, but relying on compiler optimization with trivial problems like this is not a good idea at all. There will be more than enough cases where compilers can't optimize for whatever reason, and there reference passing will be faster. – TravisG Jun 21 '11 at 10:16
@heishe: on the other hand, solving this "trivial" problem without relying on basic compiler optimisations has the extra cost of a memory allocation and (in the implementation presented in the question) a potential memory leak if `traverse` throws, as well as placing a burden on the calling code to correctly delete the returned pointer. – Mike Seymour Jun 21 '11 at 11:10
@heishe If you want to give the strong guarantee, the problem is not as trivial as you might think. If you pass by reference and change the state of this object it's not clear for the caller what to do with this object in case of exceptions. – hansmaad Jun 21 '11 at 11:16
Why should I not use a leading underscore for variable name? I've been using them for class/instance variables. – nf313743 Jun 21 '11 at 11:41
@johnnyturbo3 http://stackoverflow.com/questions/228783/what-are-the-rules-about-using-an-underscore-in-a-c-identifier – hansmaad Jun 21 '11 at 11:43
@johnnyturbo3: you can use a leading underscore for class member names if you really want to, as long as the second character isn't upper case, and you never use two consecutive underscores. However, you may find it easier to avoid them, rather than having to explain the rules governing reserved identifiers to people who think you're breaking them. – Mike Seymour Jun 22 '11 at 13:23

score 0 · Answer 5 · answered Jun 21 '11 at 09:59

0

It might look ugly but it's not that uncommon in some APIs to pass a double pointer.

In your case it seems unnecessary. Why does collect_file_paths not just return a pointer too?

answered Jun 21 '11 at 09:59

James

9,064
3
31
49

score 0 · Answer 6 · answered Jun 21 '11 at 10:06

If you have allocated the memory with new-operator, you can just return the pointer. Also, remember to use delete to the allocated memory. Good place for this would normally be in the destructor method (not in this case, since you only use the memory in the run-method).

void ThreadWorker::run()
{
  QStringList* file_list;
  file_list = ds.get_file_names(_strPath);

  //here we do something with file_list
  //...

  //Free the memory. You have to do this if the object pointed by file_list is not
  //used anywhere else.
  delete file_list;
}

QStringList* DirectorySearch::get_file_names(QString path)
{
  QStringList *file_names = new QStringList;
  traverse(path, file_names);
  return file_names;
}

score 0 · Answer 7 · answered Jun 21 '11 at 10:58

Assuming that the user has access to the header but not to the implementation, he/she hasn't got any idea how to handle the pointer. If your function is a source (aka allocate the pointer with new) you should return an auto_ptr. auto_ptr are: standard, never throws and specifically design to do this job.

You may have a look at this http://www.gotw.ca/publications/using_auto_ptr_effectively.htm

Recommended way of returning pointer from C++ function?

7 Answers7