scoped_connection: new wrapper to auto-disconnect…

[db-src]

…a contained sigc::connection, when the scoped_connection is destructed.

#87

[db-src]

Updated to be final, add .release() -> connection, and make constructors [[nodiscard]] since it would not be a good idea to accidentally create a scoped_connection and immediately discard+disconnect it.

One thing I'm not 100% sure about is the noexcept specifiers I've added. I think the existing sigc::connection copy members could be noexcept, since they just copy the weak_raw_ptr<slot>, and that itself is noexcept - but I guess they can't be until we can break ABI. And I might be wrong anyway. Please can you double-check my assumptions regarding noexceptness of both old and new connection classes?

[kjellahl]

New files shall also be added to

• sigc++/sigc++.h
• sigc++/filelist.am
• tests/Makefile.am

Add @newin{3,6} in the class description.

In the code snippet showing shared ownership, shouldn't it be

auto shconn = std::make_shared<sigc::scoped_connection>(sig.connect(&some_function));

In the description of the move constructor, remove "The current slot, if any...".
There is no current slot in a constructor.

../sigc++/scoped_connection.cc:80:31: warning: unused parameter ‘should_block’ [-Wunused-parameter]
   80 | scoped_connection::block(bool should_block) noexcept
      |                          ~~~~~^~~~~~~~~~~~

In the .cc file, s/return conn_.block();/return conn_.block(should_block);/

You seem to assume that sigc::connection is movable. It is not. It's copyable
but not movable.

How is swap() supposed to work? It's declared as a friend in .h, defined in .cc,
but no declaration is seen from .h.
Its (and release()'s) definition contains tab characters, which we try to avoid.
What's using std::swap; good for? It's more obvious which swap() is used if
s/swap(sca.conn_, scb.conn_);/std::swap(sca.conn_, scb.conn_);/.

I tried to add a call to swap() in test_scoped_connection.cc. Results:

1. std::swap(sconfoo, sconbar); works. std::swap() is called.
2. sigc::swap(sconfoo, sconbar); does not compile.

   ../tests/test_scoped_connection.cc:87:11: error: ‘swap’ is not a member of ‘sigc’
      87 |     sigc::swap(sconfoo, sconbar);
         |           ^~~~
   

3. `swap(sconfoo, sconbar);` works. sigc::swap() is called.

I don't understand why case 3 works.

`noexcept` is used inconsistently. For instance, `connection(const connection& c)`
is not `noexcept` although it only calls `weak_raw_ptr(const weak_raw_ptr& src)`,
which is `noexcept`, although it calls `trackable::add_destroy_notify_callback()`,
which is not `noexcept`. I think the only call that can throw anything is a call
to std::list::emplace_back() in trackable_callback_list::add_callback().
It can theoretically throw std::bad_alloc. I don't think you need to bother about it.
You can keep the `noexcept` declarations in scoped_connection.

[db-src]

Thanks for the other spots, will address later.

---

You seem to assume that sigc::connection is movable. It is not. It's copyable

I don't assume that. I saw that it is only copyable right now. But if it were to be movable in future, we could have newer code ready to take advantage of that. But if you want me to drop the moves, let me know.

---

How is swap() supposed to work?

I really should have tested that. I'll add a test and get it working right.

What's using std::swap; good for?

I believe this is the usually recommended 'swap two-step', where we ensure that we'll use std::swap() as a fallback, but if e.g. sigc::connection declared a customised swap() that would get preferred. Does that make sense here?

[db-src]

swap() is now implemented and tested. Let me know if you want anything else changed! Thanks for the review!

[kjellahl]

What's using std::swap; good for?

Your explanation makes sense.

std::move() or not std::move()?

I reacted to pass-by-value in the constructor and assignment taking a connection.
I thought it would mean the connection is copied twice, when it's not movable.
Now I've reread Effective Modern C++ by Scott Meyers, item 41 Consider pass by
value for copyable parameters that are cheap to move and always copied, so
I understand why to chose pass by value. When I tested with gdb and gcc 12.2.0,
I found that only one copy is performed. The compiler optimizes away the
unnecessary copy. Let's keep it as is.

There are a few minor fixes to do. I'll fix them myself after I've merged your PR.
That's easier than explaining what I want done.

Probably the clang-format CI test will react to your code. If it does react
to neatly formatted code, I'll remove that CI test. It's too nitpicking.
It's been a nuisance several times.

[db-src]

Thanks for the merge and cleanups!

Do you think there's anywhere else we could/should add mentions of sigc::scoped_connection? I'm not too familiar with the sigc++ docs.

[kjellahl]

You can see the latest documentation at https://libsigcplusplus.github.io/libsigcplusplus/
and links from there.

If you really want to document more, you can mention scoped_connection at
https://libsigcplusplus.github.io/libsigcplusplus/manual/html/sect-disconnecting.html
but I don't think it's important.
(Source code at libsigc++/docs/docs/manual/libsigc_manual.xml)