Page MenuHomePhabricator

[libcxx] Library support for contracts (C++2a)
Needs ReviewPublic

Authored by hamzasood on Jul 22 2018, 5:35 AM.

Details

Summary

This patch adds the library components needed for contracts in C++2a.

The wording says that a contract_violation object is populated in an implementation-defined manner. A private constructor that the compiler can call seems like the most sensible (and obvious) solution.

Diff Detail

Event Timeline

hamzasood created this revision.Jul 22 2018, 5:35 AM
mclow.lists added a comment.EditedJul 22 2018, 5:30 PM

Thanks for doing this, but it needs tests before it can land.
If I were to define class contract_violation {}; that would pass the tests you've provided.
(Heck, if I removed the entire definition of contract_violation, it would still pass.)

include/contract
42

Needs _LIBCPP_TYPE_VIS

I get that lib++ will not make these things, but we still need some tests showing that the whole machinery works.

hamzasood updated this revision to Diff 156808.Jul 23 2018, 9:54 AM
  • Added a missing visibility attribute.
  • Added a simple test that type-checks the expected public interface.

Testing whether it's actually hooked up correctly is quite problematic; it isn't meant to be instantiable. What's the best way to proceed with this?

Testing whether it's actually hooked up correctly is quite problematic; it isn't meant to be instantiable. What's the best way to proceed with this?

The compiler is supposed to create these - right?
Does clang currently do this?

  • If not, we should wait for them to catch up.
  • If so, then we should have tests that have a contract violation, and pass these around.

Since the method for hooking up a CV proc is implementation-defined, those tests will probably have to go into test/libcxx.

test/std/language.support/support.contract/contract_violation.pass.cpp
28

You can do this better with static_assert and declval:

static_assert(noexcept(std::declval<const contract_violation&>().line_number()), "");

and same with the types:

static_assert(std::is_same_v<decltype(std::declval<const contract_violation&>().line_number()>, std::uint_least32_t>, "");

(It's shorter to write using CV = const contract_violation; and then declval<CV&>())

I'd like to be clear that I'm not against using ASSERT_NOEXCEPT or ASSERT_SAME_TYPE - that's fine.
But I'd rather you use declval<> - which is only useful in an unevaluated context, rather than having a function that takes a parameter - which someone might actually call.

Also, then you can move all the tests into main and get rid of the function.

mclow.lists added inline comments.Jul 23 2018, 11:13 AM
test/libcxx/language.support/support.contract/version.pass.cpp
10

All the tests need // UNSUPPORTED: c++03, c++11, c++14, c++17

  • Made the suggested test changes.

The compiler is meant to create these; Clang doesn't do this yet but I'm working on the implementation.

I was under the impression that the compiler support would require the library stuff to land first, but I just checked Clang's coroutine tests and it looks like they include their own coroutine header so it doesn't depend on the library. I suppose the same thing can happen here, in which case I agree it's probably best to put this on hold in the meantime.