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

Improve compile error from using Gen constructor incorrectly #170

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

Conversation

cirodrig
Copy link

This change improves compile-time error reporting for a kind of error that I faced when I was learning to use rapidcheck. If Gen<T> is constructed from a lambda that returns the wrong type, the compiler would report errors in GenImpl. This change reports an error in Gen's constructor, which otherwise would not have an error.

An example where error reporting is useful:

rc::Gen<int> uniformDistribution(int limit) {
  return [limit](const rc::Random &r, int n) {
    return (int)(rc::Random(r).next() % limit); // should use rc::shrinkable::just
  };
}

@cirodrig
Copy link
Author

cirodrig commented Sep 17, 2017

The test failure log says that apt-get failed to retrieve some packages due to a network timeout. I guess the test needs to be retried later?

@janisozaur
Copy link
Contributor

@cirodrig you can force travis to redo the job if you somehow alter the PR. One simple way is simply rebasing + force push.

Project owner can manually re-trigger specific jobs and builds.

: m_impl(new GenImpl<Decay<Impl>>(std::forward<Impl>(impl))) {
static_assert(MakesShrinkable<T, Decay<Impl>>::value,
"Generator implementation must have a method Shrinkable<T> operator()(const Random &, int) const");
static_assert(std::is_copy_constructible<Decay<Impl>>::value,
Copy link
Owner

Choose a reason for hiding this comment

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

It shouldn't actually need to be copyable, have you tried not having a copy constructor? I know the doc comment says it does but I think that's incorrect.

// Concept check for types that have method
// `Shrinkable<T> G::operator()(const Random &, int) const`
template<typename T, typename G, typename = void>
struct MakesShrinkable : std::false_type {};
Copy link
Owner

Choose a reason for hiding this comment

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

Wouldn't it be simpler to do:

using MakesShrinkable = std::is_convertible<decltype(std::declval<const G>()(std::declval<Random>(), 0)), Shrinkable<T>>;

(Save for possible typos on my part)

Copy link
Author

Choose a reason for hiding this comment

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

Didn't realize I could do that. Made both changes.

…atible type

This produces a compile error in Gen's constructor.  Before this change,
errors would be attributed to GenImpl.
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 this pull request may close these issues.

3 participants