Skip to content

Prefer T(...) over T{...} when they behave differently in construct, makeUnique and makeShared #225

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.

Sign up for GitHub

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

Open
stolyaroleh wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Prefer T(...) over T{...} when they behave differently in construct, makeUnique and makeShared #225

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions include/rapidcheck/detail/ApplyTuple.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ struct ApplyTupleImpl;

template <typename... Ts, std::size_t... Indexes, typename Callable>
struct ApplyTupleImpl<std::tuple<Ts...>, Callable, IndexSequence<Indexes...>> {
using ReturnType = typename std::result_of<Callable(Ts &&...)>::type;
using ReturnType = typename std::result_of<Callable&&(Ts &&...)>::type;
Copy link
Contributor

@Thorius Thorius Dec 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clever fix of the compilation issue. I guess the non-SFINAE friendliness of result_of, coupled with the fact that it is not defined for function types (but fine for function references; source: https://en.cppreference.com/w/cpp/types/result_of#Notes ) cause the compilation error.

Copy link
Owner

@emil-e emil-e Jan 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid I'm not following the reasoning for making this an rvalue here. It's not called as an rvalue below so I don't see how this is correct?

Copy link
Contributor Author

@stolyaroleh stolyaroleh Jan 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::result_of<F(Args...)>::type does not work if F is a function (but works if F is a reference to function).
Here's an example:

int foo(int x) { return x; }
using T = std::result_of<decltype (foo)(int)>::type;

^ fails with

./foo.cpp:34:39: error: function cannot return function type 'decltype(foo)' (aka 'int (int)')

...however, this works:

using T = std::result_of<decltype (&foo)(int)>::type;

When I do:

template <typename T, typename... Args>
Gen<T> construct(Gen<Args>... gens) {
  return gen::map(gen::tuple(std::move(gens)...),
                  [](std::tuple<Args...> &&argsTuple) {
                    return rc::detail::applyTuple(
                        std::move(argsTuple),
                        [](Args &&... args) { return T{std::move(args)...}; });
                      std::forward<std::tuple<Args...>>(argsTuple),
                      detail::construct<T, Args...>
                    );
                  });
}

...it fails when it tries to do result_of<decltype(detail::construct<T, Args...>)(Args...), while I want it to do result_of<decltype(&detail::construct<T, Args...>)(Args...).

https://en.cppreference.com/w/cpp/types/result_of#Notes suggests using result_of<F&&(Args...)> instead result_of<F(Args...)> to avoid this.

Copy link
Contributor Author

@stolyaroleh stolyaroleh Jan 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, one might also wonder whether putting & in front of detail::construct<T, Args...> will work. Unfortunately, I never managed to check it since it crashes Clang 3.5.

Copy link
Contributor

@Thorius Thorius Jan 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of the limitations of result_of I think it is better to keep the &&. Technically speaking this is not a r-value reference, but does reference collapsing based on the value category of Callable so there should be no substantial changes.


static ReturnType apply(std::tuple<Ts...> &&tuple, Callable &&callable) {
return callable(std::move(std::get<Indexes>(tuple))...);
Expand All @@ -24,7 +24,7 @@ template <typename... Ts, std::size_t... Indexes, typename Callable>
struct ApplyTupleImpl<std::tuple<Ts...> &,
Callable,
IndexSequence<Indexes...>> {
using ReturnType = typename std::result_of<Callable(Ts &...)>::type;
using ReturnType = typename std::result_of<Callable&&(Ts &...)>::type;

static ReturnType apply(std::tuple<Ts...> &tuple, Callable &&callable) {
return callable(std::get<Indexes>(tuple)...);
Expand All @@ -35,7 +35,7 @@ template <typename... Ts, std::size_t... Indexes, typename Callable>
struct ApplyTupleImpl<const std::tuple<Ts...> &,
Callable,
IndexSequence<Indexes...>> {
using ReturnType = typename std::result_of<Callable(const Ts &...)>::type;
using ReturnType = typename std::result_of<Callable&&(const Ts &...)>::type;

static ReturnType apply(const std::tuple<Ts...> &tuple, Callable &&callable) {
return callable(std::get<Indexes>(tuple)...);
Expand Down
58 changes: 47 additions & 11 deletions include/rapidcheck/gen/Build.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#pragma once

#include <memory>

#include "rapidcheck/gen/Tuple.h"
#include "rapidcheck/detail/ApplyTuple.h"

Expand Down Expand Up @@ -119,15 +121,51 @@ class BuildMapper<T, rc::detail::IndexSequence<Indexes...>, Lenses...> {
std::tuple<Lenses...> m_lenses;
};

template<typename T, typename... Args>
typename std::enable_if<std::is_constructible<T, Args...>::value, T>::type
construct(Args &&... args) {
return T(std::forward<Args>(args)...);
}

template<typename T, typename... Args>
typename std::enable_if<!std::is_constructible<T, Args...>::value, T>::type
construct(Args &&... args) {
return T{std::forward<Args>(args)...};
}

template<typename T, typename... Args>
typename std::enable_if<std::is_constructible<T, Args...>::value, std::unique_ptr<T>>::type
makeUnique(Args &&... args) {
return std::unique_ptr<T>(new T(std::forward<Args>(args)...));
}

template<typename T, typename... Args>
typename std::enable_if<!std::is_constructible<T, Args...>::value, std::unique_ptr<T>>::type
makeUnique(Args &&... args) {
return std::unique_ptr<T>(new T{std::forward<Args>(args)...});
}

template<typename T, typename... Args>
typename std::enable_if<std::is_constructible<T, Args...>::value, std::shared_ptr<T>>::type
makeShared(Args &&... args) {
return std::make_shared<T>(std::forward<Args>(args)...);
}

template<typename T, typename... Args>
typename std::enable_if<!std::is_constructible<T, Args...>::value, std::shared_ptr<T>>::type
makeShared(Args &&... args) {
return std::shared_ptr<T>(new T{std::forward<Args>(args)...});
Copy link
Contributor

@Thorius Thorius Dec 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cannot work the same way as above (using std::make_shared) but an alternative would be to do:

return std::make_shared(T{std::forward<Args>(args)...});

This would do one allocation but it would have to move the aggregate in the memory (which would likely be a copy since aggregates are mostly simple data structs). I believe it would still be more efficient than what we have now (but not as perfect as the other case above).

Copy link
Contributor Author

@stolyaroleh stolyaroleh Dec 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will require checks for std::is_copy_constructible and std::is_move_constructible, since it's possible to have aggregates that are not copyable/movable. @emil-e, do you think it's worth adding extra overloads?

Copy link
Owner

@emil-e emil-e Jan 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I think it's worth it. This solution is not the absolutely most performant but it's the most compatible in combination with relative simplicity.

Copy link
Contributor

@Thorius Thorius Jan 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it might be better to not strive for absolute generality for the sake of simplicity. And yeah, it will probably be completely insignificant in the larger picture.

}
} // namespace detail

template <typename T, typename... Args>
Gen<T> construct(Gen<Args>... gens) {
return gen::map(gen::tuple(std::move(gens)...),
[](std::tuple<Args...> &&argsTuple) {
return rc::detail::applyTuple(
std::move(argsTuple),
[](Args &&... args) { return T{std::move(args)...}; });
std::forward<std::tuple<Args...>>(argsTuple),
detail::construct<T, Args...>
);
});
}

Expand All @@ -141,22 +179,20 @@ Gen<std::unique_ptr<T>> makeUnique(Gen<Args>... gens) {
return gen::map(gen::tuple(std::move(gens)...),
[](std::tuple<Args...> &&argsTuple) {
return rc::detail::applyTuple(
std::move(argsTuple),
[](Args &&... args) {
return std::unique_ptr<T>(new T{std::move(args)...});
});
std::forward<std::tuple<Args...>>(argsTuple),
detail::makeUnique<T, Args...>
);
});
}

template <typename T, typename... Args>
Gen<std::shared_ptr<T>> makeShared(Gen<Args>... gens) {
return gen::map(gen::tuple(std::move(gens)...),
[](std::tuple<Args...> &&argsTuple) {
return rc::detail::applyTuple(std::move(argsTuple),
[](Args &&... args) {
return std::make_shared<T>(
std::move(args)...);
});
return rc::detail::applyTuple(
std::forward<std::tuple<Args...>>(argsTuple),
detail::makeShared<T, Args...>
);
});
}

Expand Down
43 changes: 43 additions & 0 deletions test/gen/BuildTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include <catch2/catch.hpp>
#include <rapidcheck/catch.h>

#include "util/Aggregate.h"
#include "util/GenUtils.h"
#include "util/Predictable.h"
#include "util/Logger.h"
Expand Down Expand Up @@ -90,6 +91,20 @@ TEST_CASE("gen::construct") {
RC_ASSERT(isArbitraryPredictable(std::get<0>(value)));
RC_ASSERT(isArbitraryPredictable(std::get<1>(value)));
}

SECTION("works with aggregate initializers") {
gen::construct<NestedAggregate>(gen::arbitrary<Aggregate>(),
gen::arbitrary<int>());
gen::construct<NestedAggregate>(gen::arbitrary<int>(),
gen::arbitrary<int>(),
gen::arbitrary<int>());
}

SECTION("prefers T(x, y) over T{x, y} when they behave differently") {
auto a = gen::construct<std::vector<int>>(gen::just(3), gen::just(3))(Random(), 0).value();
std::vector<int> b(3, 3);
RC_ASSERT(a == b);
}
}

TEST_CASE("gen::makeUnique") {
Expand Down Expand Up @@ -122,6 +137,20 @@ TEST_CASE("gen::makeUnique") {
RC_ASSERT(isArbitraryPredictable(std::get<0>(value)));
RC_ASSERT(isArbitraryPredictable(std::get<1>(value)));
}

SECTION("works with aggregate initializers") {
gen::makeUnique<NestedAggregate>(gen::arbitrary<Aggregate>(),
gen::arbitrary<int>());
gen::makeUnique<NestedAggregate>(gen::arbitrary<int>(),
gen::arbitrary<int>(),
gen::arbitrary<int>());
}

SECTION("prefers T(x, y) over T{x, y} when they behave differently") {
auto a = gen::makeUnique<std::vector<int>>(gen::just(3), gen::just(3))(Random(), 0).value();
auto b = std::unique_ptr<std::vector<int>>(new std::vector<int>(3, 3));
RC_ASSERT(*a == *b);
}
}

TEST_CASE("gen::makeShared") {
Expand Down Expand Up @@ -154,6 +183,20 @@ TEST_CASE("gen::makeShared") {
RC_ASSERT(isArbitraryPredictable(std::get<0>(value)));
RC_ASSERT(isArbitraryPredictable(std::get<1>(value)));
}

SECTION("works with aggregate initializers") {
gen::makeShared<NestedAggregate>(gen::arbitrary<Aggregate>(),
gen::arbitrary<int>());
gen::makeShared<NestedAggregate>(gen::arbitrary<int>(),
gen::arbitrary<int>(),
gen::arbitrary<int>());
}

SECTION("prefers T(x, y) over T{x, y} when they behave differently") {
auto a = gen::makeShared<std::vector<int>>(gen::just(3), gen::just(3))(Random(), 0).value();
auto b = std::shared_ptr<std::vector<int>>(new std::vector<int>(3, 3));
RC_ASSERT(*a == *b);
}
}

namespace {
Expand Down
23 changes: 23 additions & 0 deletions test/util/Aggregate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
#pragma once

#include "rapidcheck.h"

namespace rc {
struct Aggregate {
int x;
int y;
};

template<>
struct Arbitrary<Aggregate> {
static Gen<Aggregate> arbitrary() {
return gen::construct<Aggregate>(gen::arbitrary<int>(),
gen::arbitrary<int>());
}
};

struct NestedAggregate {
Aggregate a;
int z;
};
} // namespace rc