1

I am trying to use unique_ptr instead of allocating memory myself. I have the following code:

class Album {
...
public:
    Add(Song* song);
...
}

void func(){
    ...
    std::unique_ptr<Album> album = std::unique_ptr<Album>{new Album()};
    std::unique_ptr<Song> song = std::unique_ptr<Song>{new Song(soundtrack.data(), soundtrack.length())};
    album->Add(song.get());
    ...
}

I get segmentation fault for the line:

album->Add(song.get());

I tried multiple variations to get the pointer, including std::move and make_unique, but maybe I don't understand how unique_ptr works well enough to solve it.

Any ideas?

O.B.
  • 29
  • 1
  • 7
  • 2
    Does that even compile? A member function needs a return type. – acraig5075 Nov 21 '19 at 08:07
  • 2
    `Add` should probably take `std::unique_ptr` and not `Song*`. Then you'll have to `std::move`: `album->Add(std::move(song));`. – Evg Nov 21 '19 at 08:10
  • @Evg not neccessarily: If you're not taking ownership it's completely reasonable to take a raw pointer as argument. See also this [GotW](https://herbsutter.com/2013/06/05/gotw-91-solution-smart-pointer-parameters/). – andreee Nov 21 '19 at 08:14
  • 1
    @andreee, absolutely! But the the logic here suggests that it is `Album` that is responsible for songs, not `func`. – Evg Nov 21 '19 at 08:16
  • 7
    @O.B. I think you need to provide more information. Most probably your `album` is outliving the `song` instance, due to a misuse of `unique_ptr` (as mentioned by Evg). Side note: [You should prefer](https://stackoverflow.com/questions/22571202/differences-between-stdmake-unique-and-stdunique-ptr-with-new) `make_unique` over calling `new` in the ctor. – andreee Nov 21 '19 at 08:19
  • 4
    The point of the smart "pointers" is not managing memory but managing *ownership*; they are almost entirely dissimilar from actual pointers. They are not magic pixie dust that saves you from thinking about lifetimes and owners. Using a raw pointer that you acquired with `get` suffers from many of the same problems as using one you acquired with `&`. – molbdnilo Nov 21 '19 at 08:29
  • 1
    @O.B. maybe _you_ should read the [GotW](https://herbsutter.com/2013/06/05/gotw-91-solution-smart-pointer-parameters/) I referred to in my previous comment, it should help you to gain a better understanding of smart pointers and how to use them properly. In particular Guru question 3. – andreee Nov 21 '19 at 10:13
  • @acraig5075 yes my mistake not including it. This is part of a much bigger code. – O.B. Nov 21 '19 at 10:47
  • @andreee I will take a look at it. I do not want to change the signature of the function. Maybe my usuage of unique_ptr is incorrect. What I tried to achieve is to not manage the memory of the song and album, and de-allocate songs when they are removed from album (no more owners). – O.B. Nov 21 '19 at 10:52
  • @O.B. you may take a look at the `unique_ptr::release` function then. It allows you to transform a unique pointer into an owning raw pointer. – Guillaume Racicot Nov 28 '19 at 14:09

2 Answers2

1

The problem is as follows

class Album {
...
public:
    Add(Song* song);
...
}

void func(){
    ...
    std::unique_ptr<Album> album = std::unique_ptr<Album>{new Album()};
    std::unique_ptr<Song> song = std::unique_ptr<Song>{new Song(soundtrack.data(), soundtrack.length())};
    album->Add(song.get());
    ...
    // Here the object song gets destructed. This means that the underlying Song gets destructed.
    // So right after leaving func() the pointer that was returned by song.get() now points to non-allocated memory containing random bits at worst case.
}

So one possible solution would be...

class Album {
...
public:
    Add(std::unique_ptr<Song> song); // you still need to move song inside Add(...)
...
}

void func(){
    ...
    std::unique_ptr<Album> album = std::unique_ptr<Album>{new Album()};
    std::unique_ptr<Song> song = std::unique_ptr<Song>{new Song(soundtrack.data(), soundtrack.length())};
    album->Add(std::move(song)); //here song is intended  to be moved inside Add(...)
    ...
    // song points to nullptr here.
}
Necktschnagge
  • 341
  • 1
  • 3
  • 13
1

The code you provided compiles and runs fine - thus there must be a problem in the part you didnot provide - I suspect code inside Add() or it's returntype, or some later use of the pointers as necktschnagge suspected. Working example is on gdbonline:
https://onlinegdb.com/r1oyXGK2S

First of all I ask the question, what is the advantage you'll like to achive by use of std::unique_ptr. Consider that a unique pointer does not guarantee to have a pointee - inside Add() you have to check for nullptr! I think from your usage, you do not want to use a std::unique_ptr:

Key is, that a std::unique_ptr has just a unique ownership. either one of those:

  • func()::local scope
  • album::Add()::parameter_scope

ownes it.

Since you didn't use std::move() the ownership remains in func(), and will be destroyed by the end of func(). To avoid this you might as well use song.release() (see cpp-reference).

Xantopp
  • 64
  • 7