Std::list

Hello.
I’m trying to replace the deprecated std::list<ScopedPointer > markers;
to std::list<std::unique_ptr > markers;
.hpp:

void WaveMarkerComp::addMarkerToList(double time, const String &title, const String &channel, const String &note, bool saveXML)
{
  MarkerInfo *newMarker = new MarkerInfo(time, title, channel, note);
  //MarkerInfo *newMarker (new MarkerInfo(time, title, channel, note));
  newMarker->channelBtn.removeListener (this);
  newMarker->noteBtn.removeListener (this);
  newMarker->delMarker.addListener(this);
  newMarker->editMarkerBtn.addListener(this);
  newMarker->channelBtn.addListener(this);
  newMarker->noteBtn.addListener(this);

  addChildComponent(newMarker);
  markers.push_back(newMarker); // !!!!!!!!!!!!!!!!!!!!!!
  resized();
  if (saveXML)
    saveMarkers();
}

unable to convert “MarkerInfo *” to “_Ty”
I ask for help please. Help with initialization.

You shouldn’t create a raw pointer with new. Instead, do something like:

auto& newMarker = markers.push_back (std::make_unique<MarkerInfo>(time, title, channel, note));

newMarker->channelBtn.removeListener(this);
...

I wish that would work, but std::vector push_back returns void.

Edit: I just see it’s a bout std::list push_back. But that returns void as well.

Thank You. But compieler says:
…/Source/MainComponent.cpp:193:29: No matching member function for call to ‘push_back’
…/Source/MainComponent.cpp:193:7: Cannot form a reference to ‘void’
I will search further.

Ah sorry, I forgot about that. Do push_back and then retrieve it using back()

Read on C++ sites about the differences between push_back and emplace_back and prefer the latter.

Cheers

I would disagree here. In case of a vector/list of unique_ptr in conjunction with std::make_unique, push_back is the right choice. The point of emplace_back is to avoid an unnecessary temorary copy of an object, but for a type like unique_ptr that has a deleted copy constructor and only a move constructor, creating the instance inside make_unique and implicitly moving it into the container in push_back is the most straightforward way of expressing what you are actually doing in your code in my opinion. Why would you chose emplace back here?

3 Likes

It’s more a general remark, since I see a lot of older-C++ coding styles on the forum where people don’t seem to be aware of methods like this.
emplace_back is a good default choice to avoid unnecessary copies in general imho. And it has no disadvantages compared with push_back.

Kind of awareness alert :wink:

With emplace_back you don’t need back (), for instance. It returns a ref, referring to the remarks by @daniel and @benvining

I’ve been down this rabbit hole!

emplace_back constructs an object in place with the arguments forwarded as an rvalue reference template parameter pack. Emplace vs Push is less of an issue with non-copyable objects like std::unique_ptr, as trying to pass by value will cause the compiler to explode anyway.

This is perfectly fine:

    container.emplace_back (new MarkerInfo);
    container.emplace_back (std::make_unique<MarkerInfo>());

Although I would personally do:

    auto marker = std::make_unique<MarkerInfo>();

    // do things to/with 'marker'

    // this will likely call 'emplace_back' anyway but clearly shows intent
    container.push_back (std::move (marker));

    // and if you want a reference to it
    auto& markerRef = container.back();

^^ Clearly describes who owns things.

std::container::*_back returning a reference is a c++17 thing, so be careful if you plan to support older compilers.

1 Like

Ah, the “joy” of having to think about older compilers… eek :grimacing:

(never thought about that yet, because I don’t have to)

1 Like

Issue here is you now have an empty marker object floating around, you should reduce the scope!

Either do

container.push_back (std::make_unique<MarkerInfo>());
auto& marker = container.back();

// do things to/with 'marker'

Or

{
    auto marker = std::make_unique<MarkerInfo>();

    // do things to/with 'marker'

    container.push_back (std::move (marker));
}

auto& marker = container.back();

Or, even better, do the setup in a factory function:

container.push_back (createMarker (*this, other, params));
1 Like

const auto& markerRef = container.emplace_back (std::make_unique <MarkerInfo> ());

Still not convinced of the preference for the older push_back () and back () combination,

ok, two assignments in one statement, maybe not be my favorite as well, but then again, I’m also used to the addAndMakeVisible patterns

They’re functionally the same. const auto& won’t work with std::unique_ptr, though.

I would use the one-liner if I didn’t have to worry about backwards compatibility.

EDIT: Sorry! const auto& is legal. I was thinking of copy semantics, which std::unique_ptr doesn’t support.

Why would that not work?

Just my two-penneth… :slight_smile:

Totally agree with ImJimmy about not creating a std::unique_ptr variable and then moving out of it. Leaving an empty pointer in scope for no good reason is a bit untidy.

I tend to not expect push_back() to return the reference… Although I’m on c++17, doing that always seems to cause problems. But maybe compilers have caught up a bit now and it’s something I should get more into the habit of doing.

Anyway, my suggestion would be this:

container.push_back (std::make_unique<MarkerInfo>());
auto& marker = *container.back();

The thing I’m adding to the debate is the defreference of the std::unique_ptr so that the marker variable is a reference to the object itself, and not to the pointer.

That’s really important for a couple of reasons:

  • your code will be neater because you’ll be using . rather than -> when you use it.
  • If the code that follows this mutates the vector, then this reference will still be valid. But if you have a reference to the pointer inside the vector, it’ll be a dangling pointer. This is the kind of bug that you get when you write code that works, and then someone comes along later and adds a bit more code that mutates the vector without noticing this, so it’s best to write your original code defensively.
5 Likes

Hi Jules.
Only this solution is suitable, but the compiler throws an error.

markers.push_back (std::make_unique<MarkerInfo>(time, title, channel, note));
  auto& newMarker = markers.back();
  newMarker->channelBtn.removeListener (this);
  newMarker->noteBtn.removeListener (this);
  newMarker->delMarker.addListener(this);
  newMarker->editMarkerBtn.addListener(this);
  newMarker->channelBtn.addListener(this);
  newMarker->noteBtn.addListener(this);

  addChildComponent(newMarker); //!!! Compieler: No matching member function for call to 'addChildComponent'

Without it, markers are not visible.
P.S. I’m currently using C++14 as I don’t have time to upgrade from El Capitan to a later OS.

addChildComponent(newMarker.get());

addChildComponent has two overloads:

void addChildComponent (Component* child, int zOrder=-1)
void addChildComponent (Component& child, int zOrder=-1)

So you can either pass a reference or a raw pointer. A std::unique_ptr is not implicitly convertible to any of the two versions, this was different with juce::ScopedPointer that was implicitly convertible to its underlying pointer. But you can explicitly obtain the underlying raw pointer via get() or dereference it via * like:

addChildComponent (newMaker.get()); // Pointer overload
addChildComponent (*newMarker); // Reference overload

Both are technically correct, I personally prefer the second version.

Didn’t think. Thank you. This thread has been a great experience for me.

You replied to me, but you seem to have ignored the suggestion that I posted! My point was that you should use a reference to the pointer, not the pointer itself. If you did that, you wouldn’t have got confused about how to use the pointer, and your code would have been neater without all the -> operators

1 Like