Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Variadic constructor of indirect is too greedy #159

Closed
Ukilele opened this issue Nov 10, 2023 · 1 comment · Fixed by #186
Closed

Variadic constructor of indirect is too greedy #159

Ukilele opened this issue Nov 10, 2023 · 1 comment · Fixed by #186

Comments

@Ukilele
Copy link
Collaborator

Ukilele commented Nov 10, 2023

In PR #128 we removed the parameter std::in_place_t from the variadic constructor of indirect:

template <class... Ts>
- explicit constexpr indirect(std::in_place_t, Ts&&... ts);
+ explicit constexpr indirect(Ts&&... ts);

The following snippet shows a simplified version of indirect and a (potentially contrived) scenario, where the variadic constructor might be too greedy:

#include <concepts>

template<class T>
struct indirect {
    indirect() = default;

    template<class ...Ts>
    explicit indirect(Ts&&... ts)
        requires std::constructible_from<T, Ts...> {}
};

struct Evil {
    Evil(indirect<Evil>&) {}
};

int main() {
    indirect<Evil> i1;
    indirect<Evil> i2(i1);
}

https://compiler-explorer.com/z/KWvzcnjY9

In the line indirect<Evil> i2(i1); we don't call the copy constructor of indirect (as I would expect as a user of indirect), but we call the variadic constructor as it is a better match. Note that it is only a better match as i1 is non-const. For a const indirect<Evil> the copy-constructor would still be called.
The first question is whether you all agree with me that indirect<Evil> i2(i1); should indeed call the copy-constructor or if you think that it is intentional design that it calls the variadic constructor. And if you agree, I think a fix would be to constraint the variadic constructor further. Something like:

template <class... Ts>
explicit constexpr indirect(Ts&&... ts)
- requires std::constructible_from<T, Ts&&...>;
+ requires (std::constructible_from<T, Ts...> &&
+     ( sizeof...(Ts) != 1 || !(std::same_as<std::remove_reference_t<Ts>, indirect> || ...) )
+ );

In prose: If the pack Ts consists of only one type U, make sure that U is not indirect&.

@jbcoe
Copy link
Owner

jbcoe commented Nov 10, 2023

I agree that this is an issue and that your fix is good. Are you happy to raise a PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants