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

stolyaroleh · 2018-12-14T12:09:58Z

@Thorius reviewed my earlier PR (#224) and brought to my attention that it changed the behaviour of gen::construct<T>(...) when T has a constructor that accepts an initializer list. One such example:

vector<int>(3, 0); // [0, 0, 0]
vector<int>{3, 0}; // [3, 0]

I think it's sensible to expect gen::construct, gen::makeUnique and gen::makeShared to act like the corresponding factory functions from the standard library when such ambiguity arises. After some searching I found this paper: http://open-std.org/JTC1/SC22/WG21/docs/papers/2015/n4462.html, describing one possible way to make aggregates work without breaking existing behaviour.

This PR makes gen::construct, gen::makeUnique and gen::makeShared check whether object is_constructible, and only fall back to brace initialization when it's not. I also added tests for each of them.

stolyaroleh · 2018-12-14T22:57:20Z

...sadly, this does not compile on Clang 3.5 :/
I managed to reproduce locally, will try to fix.

Thorius

I think you can try some different version of SFINAE (default template parameter or default non-type template parameter). As a last resort, you can try tag dispatch with something like std::true_type and std::false_type.

include/rapidcheck/gen/Build.hpp

Thorius

Besides the couple of improvements suggested, this looks great!

Thorius · 2018-12-23T21:04:36Z

include/rapidcheck/detail/ApplyTuple.h

@@ -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;


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.

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?

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.

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.

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.

include/rapidcheck/gen/Build.hpp

Thorius · 2018-12-23T21:20:33Z

include/rapidcheck/gen/Build.hpp

+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)...});


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).

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?

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.

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.

test/util/Aggregate.h

...in gen::construct and gen::makeUnique. Also, add support for aggregate initialization in gen::makeShared and tests for all of them.

emil-e

Sorry for not getting back earlier, life gets in the way!

This looks great, but I'm a bit confused about the return type stuff so if you could clarify that I would appreciate it.

emil-e · 2019-01-19T11:15:17Z

include/rapidcheck/detail/ApplyTuple.h

@@ -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;


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?

emil-e · 2019-01-19T11:18:49Z

include/rapidcheck/gen/Build.hpp

+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)...});


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.

stolyaroleh force-pushed the master branch from 468a418 to 92cd8d3 Compare December 14, 2018 12:36

Thorius reviewed Dec 20, 2018

View reviewed changes

include/rapidcheck/gen/Build.hpp Outdated Show resolved Hide resolved

stolyaroleh force-pushed the master branch from 92cd8d3 to 0e25dfa Compare December 23, 2018 17:43

Thorius reviewed Dec 23, 2018

View reviewed changes

Prefer T(...) over T{...} when they behave differently

b6c7511

...in gen::construct and gen::makeUnique. Also, add support for aggregate initialization in gen::makeShared and tests for all of them.

stolyaroleh force-pushed the master branch from 0e25dfa to b6c7511 Compare December 23, 2018 23:43

emil-e requested changes Jan 19, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

stolyaroleh commented Dec 14, 2018

stolyaroleh commented Dec 14, 2018

Thorius left a comment

Thorius left a comment

Thorius Dec 23, 2018

emil-e Jan 19, 2019

stolyaroleh Jan 21, 2019 •

edited

Loading

stolyaroleh Jan 21, 2019 •

edited

Loading

Thorius Jan 24, 2019

Thorius Dec 23, 2018

stolyaroleh Dec 23, 2018

emil-e Jan 19, 2019

Thorius Jan 24, 2019

emil-e left a comment

emil-e Jan 19, 2019

emil-e Jan 19, 2019

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

Are you sure you want to change the base?

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

Conversation

stolyaroleh commented Dec 14, 2018

stolyaroleh commented Dec 14, 2018

Thorius left a comment

Choose a reason for hiding this comment

Thorius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stolyaroleh Jan 21, 2019 • edited Loading

Choose a reason for hiding this comment

stolyaroleh Jan 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emil-e left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stolyaroleh Jan 21, 2019 •

edited

Loading

stolyaroleh Jan 21, 2019 •

edited

Loading