Look into using clang-tidy

[ispeters]

@ccotter suggested on PR #545 that clang-tidy might be able to catch the bugs we've seen with incorrect forwards, perhaps at the expense of using std::forward. Automated bug-catching is nice, and probably worth some amount of build speed degradation so we should look into some clang-tidy CI.

[ccotter]

I'd be happy to do a first pass at this and report back. One of my side hobbies it tinkering with clang-tidy and have some experience with the checks that verify correct uses of move, forward, and universal references.

[lewissbaker]

Note that on modern versions of clang std::forward/move now get translated to intrinsics which shouldn’t have the same template instantiation overhead.

[ccotter]

Ok, these two checks are of interest (both of which would appear in the next LLVM release version 17).

• cppcoreguidelines-rvalue-reference-param-not-moved
• cppcoreguidelines

Both help identify all cases where functions do not use std::move(rvalue_ref_param) or static_cast<T&&>(universal_ref_param) (or std::forward<T&&>(universal_ref_param) if you end up preferring that coding style). E.g.,

/home/ccotter/git/libunifex/include/unifex/async_mutex.hpp:119:46: warning: rvalue reference parameter 's' is never moved from inside the function body [cppcoreguidelines-rvalue-reference-param-not-moved]
  119 |     tag_invoke(tag_t<connect>, lock_sender &&s, Receiver &&r) noexcept {
      |                                              ^
/home/ccotter/git/libunifex/include/unifex/async_mutex.hpp:119:60: warning: forwarding reference parameter 'r' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward]
  119 |     tag_invoke(tag_t<connect>, lock_sender &&s, Receiver &&r) noexcept {

Both checks are expecting code to always use the "named cast" using move or forward. The checks won't emit any kind of fixits though. Below are the counts of each diagnostic for all header includes:

     36 [cppcoreguidelines-rvalue-reference-param-not-moved]
    635 [cppcoreguidelines-missing-std-forward]

If you'd like, I can do a first pass to fix the parameters that use C-cast instead of std::move().

[ispeters]

Those look awesome, @ccotter. A first pass would be most welcome.

Do you know anything about running clang-tidy as a linter as part of GitHub CI because I imagine that would help us stay clean.

[ccotter]

Do you know anything about running clang-tidy as a linter

Unfortunately I'm not familiar with GitHub CI specifically. Googling around shows there's https://github.com/marketplace/actions/clang-tidy-review. What's needed generally to run clang-tidy is a build capable of producing a compile_comands.json (which cmake can do at configure time), and a .clang-tidy config file listing the checks to enable (and per-check options in some cases). Let me see if I can play with this on a personal project and see if it's easy to setup.

[ccotter]

Just to confirm, should I also look at migrating static_cast or C style forwarding casts to std::forward? I ended up writing a clang-tidy tool to do behavior preserving conversion (and it'll can optionally try to replace moves, although that transform is not possible to be behavior preserving in all cases). That said, we should still review all cases in case the intent of (T&&)t on a forwarding reference should not even be using a forwarding reference at all.

[ispeters]

should I also look at migrating static_cast or C style forwarding casts to std::forward?

Yes, please! We're having a discussion on #552 about what to do when forwarding not-really-forwarding-references, which I think makes this issue nuanced, but, for clear cases of moves and forwards, I think we should standardize on std::move and std::forward so if you're interested in doing the clean-up, I'm quite happy to review and merge.

[ispeters]

Oh, and that clang-tidy-review … thingy looks pretty useful! @janondrusek has done a lot more than I have to maintain our GitHub CI so I think you two might be best suited to discuss whether and how to add clang-tidy-review to the mix.