CODEWITHSUNDEEP
c++c++17move-semanticsclang-tidystdtuple
Sergey Kolesnik
Sergey Kolesnik

Reputation: 3640

Is Clang-Tidy correct regarding std::move when constructing std::tuple using `{}`?

I was implementing a function in C++, specifically a callable class object, designed to return a std::tuple containing a container of objects and a result. After some adjustments (see below) I have the definition of a member function:

template <typename UserGroups>
auto operator()(const UserGroups& userGroups) const noexcept
{
    using result_t = std::tuple<std::unordered_set<QnUuid>, Result>;
    State state{.inheritingGroups = {m_rootId}};
    
    for (const auto& group : userGroups)
    {
        if (Result res = dfs(state, group); !res)
            return result_t({}, std::move(res));
    }
    return result_t(std::move(state.inheritingGroups), {});
}

... where State is:

struct State
{
    std::unordered_set<QnUuid> inheritingGroups{};
    std::unordered_set<QnUuid> visited{};
    std::unordered_set<QnUuid> visiting{};
};

Clang-Tidy gives me warnings on both return statements:

Clang-Tidy: Passing result of std::move() as a const reference argument; no move will actually happen

To resolve this, I explicitly passed Result() in the second return statement:

return result_t(std::move(state.inheritingGroups), Result());

I suspect this might be due to a non-templated constructor of std::tuple being selected when {} is used as one of the arguments. Ideally, I am aiming for a simplified return syntax, such as:

// auto operator()(...) const -> std::tuple<std::unordered_set<QnUuid>, Result>

// If error occurs
if (Result res = dfs(state, group); !res)
    return {{}, std::move(res)}; // std::move is required since NRVO will not work

// On success
return {std::move(state.inheritingGroups), {}};

However, initializing the tuple with {...} results in compilation issues due to ambiguous constructor calls.

In an attempt to address this, I introduced a typedef alias, but it led to a more verbose and, in my view, less clean version:

using groups = std::unordered_set<QnUuid>;
using result_t = std::tuple<groups, Result>;

// If error occurs
if (Result res = dfs(state, group); !res)
    return result_t(groups(), std::move(res));
// On success
return result_t(std::move(state.inheritingGroups), Result());

I find the introduction of the groups alias to be a suboptimal solution as it only serves to circumvent the Clang-Tidy warning and introduces unnecessary complexity. The alias must be declared outside the immediate scope (State is used by 2 functions), thereby adding an extra layer of indirection and potentially causing future readers to look up its definition, losing the immediate clarity of it being an unordered_set.

With this context, I have two primary questions:

  1. Is Clang-Tidy accurate in stating that the move will not actually occur in this scenario?
  2. Is my suspicion regarding the selection of a non-template constructor for std::tuple when using {} as an argument correct?

I am looking for insights or alternative solutions that maintain code clarity and simplicity while properly addressing the Clang-Tidy warning. Any suggestions or explanations are highly appreciated.

Upvotes: 2

Views: 183

Answers (1)

Jan Schultke
Jan Schultke

Reputation: 39588

Yes, clang-tidy is correct. std::tuple has a lot of constructors, and because you're using {}, the forwarding constructor (which is a template) cannot be called. Only the constructor taking const Types&... can be called because no deduction of a template parameter from {} is necessary. This is constructor (2) on cppreference.

In general, no type can be deduced from {}, which is why the forwarding constructor isn't viable. See Why do auto and template type deduction differ for braced initializers?.

You can reproduce this issue as follows:

#include <tuple>

struct S {
    S(const S&);
    S(S&&);
};

std::tuple<S, int> foo(S s) {
    return std::tuple<S, int>(std::move(s), {});
}

std::tuple<S, int> bar(S s) {
    return std::tuple<S, int>(std::move(s), int{});
}

This compiles to:

foo(S):
        // ...
        call    S::S(S const&) [complete object constructor]
        // ...
bar(S):
        // ...
        call    S::S(S&&) [complete object constructor]
        // ...

See live example at Compiler Explorer.

As a workaround, you should use:

return result_t(std::move(state.inheritingGroups), Result{});
// or
return {std::move(state.inheritingGroups), Result{}};

Upvotes: 1

Related Questions