C++ API: replace bare pointer with smart pointers.

[tstenner]

As of now, each LSL C++ API class keeps a pointer to an opaque LSL object and calls a function (e.g. lsl_destroy_outlet) to deconstruct and deallocate the object in its own destructor.

This PR replaces the bare pointers with smart pointers so the compiler can keep track of the C API object lifetimes and generate copy and move constructors / assignment operators automatically.

Moreover, this makes it possible to pass the pointer to end users e.g. for calling C API functions and keep multiple copies of wrapper classes around:

std::shared_ptr<lsl::stream_outlet> raw_handle = stream_outlet(…).get_raw_handle();
// here, the stream outlet is already deconstructed, but the handle is still valid
lsl_push_foo(raw_handle.get());

auto outlet = stream_outlet(...);
auto outlet_copy = outlet;
outlet.push_chunk(...);
outlet_copy.push_chunk();

The only change in behaviour concerns stream_info objects, as the copy constructor previously performed a deep copy. This is now accomplished by the stream_info::clone() method:

// stream_info copies are cheap, for independent copies clone() has to be used
lsl::stream_info info(...);
lsl::stream_info copy1(info), copy2(info), clone(info.clone());
copy1.desc().append_child("Dummy");
REQUIRE(copy2.desc().first_child().name() == std::string("Dummy"));
REQUIRE(clone.desc().first_child().empty());

[cboulay]

This seems like a nice improvement.

I wonder about https://github.com/sccn/liblsl/blob/7c28ef4a0d0e4d6d759fedcb1130d33032429760/include/lsl_cpp.h#L868-L869 and why (&timestamp_buffer[0]) was there to begin with. It must have been deliberate. Maybe it was only to suppress a compiler warning.

My only other concern is the change in behaviour you highlighted. Do you know of any of our apps that use the C++ API and used the stream_info copy-constructor? LabRecorder seems like a good place to look.

What about taking a vector of stream_info objects, i.e. the result of lsl::resolve_streams(), then using push_back to add an element to a new vector, as used in this snippet? The docs for push_back say that the element is "copied (or moved)". Do you know if one has to explicitly use std::move for it to be moved? Or is there something implicit that decides whether it is copied or moved? (I'm wondering if the proposed change would change the outcome of such an implicit decision).

There's also this line : new std::thread(&recording::record_from_streaminfo, this, stream, true));. The value passed to the thread constructor is implicitly copied. I'm guessing that this might make use of the cheap/shallow copy here, but that's fine because if the original goes out of scope then the shallow-copy will still be valid. Is that assumption correct?

But the above question might be moot because it looks like stream is passed-by-const-reference: (line): void recording::record_from_streaminfo(const lsl::stream_info &src, bool phase_locked).

Actually now that I'm looking at that, I wonder why we don't have to use std::ref(stream) in the thread argument. You're not supposed to be able to pass-by-ref in a thread constructor without explicitly wrapping the argument in a std::ref. Are we allowed because it's a const-ref? Either way, I guess it's possible that the original stream_info goes out of scope before the info is used to spin up the outlet, so maybe this shouldn't be a by-ref and using a shallow-copy is actually an improvement.

The above concerns are probably things to fix in LabRecorder. But I'd like to make sure we're not introducing huge problems with this PR.

If the answers to the above are all "there's no difference in behaviour" then I'd be happy to merge.

[tstenner]

and why (&timestamp_buffer[0]) was there to begin with. It must have been deliberate.

Pre-C++11 the only way to "convert" an std::vector<T> to T* was to take the address of the first element (nowadays, there's std::vector<T>::data()). I think this was copied from one of the functions taking a vector of timestamps, and since it doesn't do anything nobody noticed.

My only other concern is the change in behaviour you highlighted. Do you know of any of our apps that use the C++ API and used the stream_info copy-constructor? LabRecorder seems like a good place to look.

Copying stream infos is fine, but changes made to one object will be visible in the "copies".

LabRecorder uses the copy constructor in some instances, but doesn't modify the stream infos afterwards. Just to be sure, I have added const to the stream info usages (https://github.com/labstreaminglayer/App-LabRecorder/commit/42901df6c603a48591eb188c54fc696b148090a0) where possible and removed the remaining usage (https://github.com/labstreaminglayer/App-LabRecorder/commit/5055f4d7ab27b3e6b4e929f0c054f818578b51ff).

What about taking a vector of stream_info objects, i.e. the result of lsl::resolve_streams(), then using push_back to add an element to a new vector, as used in this snippet?

Absolutely fine, unless the object is modified after putting it in the vector (i.e. for(int i=0;i<10;i++) { info.desc().set_value(…); vector.push_back(info);

The docs for push_back say that the element is "copied (or moved)". Do you know if one has to explicitly use std::move for it to be moved? Or is there something implicit that decides whether it is copied or moved? (I'm wondering if the proposed change would change the outcome of such an implicit decision).

In a nutshell, push_back will move when the argument is an rvalue (e.g. push_back(std::move(info))) and copy the argument otherwise. With the plain push_back(info), it will still copy the argument, but copying a smart pointer is much better than copying the memory pointed to it (it boils down to two atomic integer operations). Moving a smart pointer is slightly faster, but not so much it's worth worrying about in this case.

With (copy) constructors it's a bit more complicated, but I found the chapter on rvalue references and argument types in Scott Meyer's Effective Modern C++ to be quite good.

There's also this line : new std::thread(&recording::record_from_streaminfo, this, stream, true));. The value passed to the thread constructor is implicitly copied.

Ugh. As far as I understand it, this line is only valid because a copy is made implicitely (otherwise, it'd have to be std::ref(stream)). The function should take a copy of the stream info.

I'm guessing that this might make use of the cheap/shallow copy here, but that's fine because if the original goes out of scope then the shallow-copy will still be valid. Is that assumption correct?

Exactly. The stream info will be valid as long as there's at least one reference to it around, and afterwards it shouldn't be possible to refer to it.

But the above question might be moot because it looks like stream is passed-by-const-reference: (line): void recording::record_from_streaminfo(const lsl::stream_info &src, bool phase_locked).

That's actually quite bad because the thread might use the stream info after it has been deleted from the vector (or the location in memory changed due to a resizing of the vector), but as noted above a copy is made implicitely.

The above concerns are probably things to fix in LabRecorder. But I'd like to make sure we're not introducing huge problems with this PR.

After a review so thorough it even found an issue in an unrelated repository I think we're on the safe side :wink:. Many thanks for taking the time!