azure-sdk-for-cpp icon indicating copy to clipboard operation
azure-sdk-for-cpp copied to clipboard

Test each type for having the expected default operations

Open antkmsft opened this issue 3 years ago • 1 comments

It is possible to write a test helper that checks for the type to have copy constructor, move constructor, and so on. Then, for each type, we can have one extra unit test that checks whether a type has or hasn't copy constructor as expected and so on.

One way is to check it semantically, but then I think we can't test for move/vs copy and so on. But the benefit is that we can write a test that verifies that copying and assignment works exactly as we expect. Plus there's an argument that we really care about semantics and not the actual way it is implemented. But it is not possible to generalize copy constructor and assignment verfication, for each class such testcases would need to be written individually.

Second approach - and we can combine both approaches at the same time - second approach, would be to write a templated test helper to which you tell what operations you expect the type to have, and it will verify that your expectations are true. We can do so without a test helper, and just repeat most of the statements for each class, but still the sense is the same - to use type_traits.

Check out what type_traits have: https://en.cppreference.com/w/cpp/header/type_traits

  • is_constructible

  • is_trivially_constructible

  • is_nothrow_constructible

  • is_default_constructible

  • is_trivially_default_constructible

  • is_nothrow_default_constructible

  • is_copy_constructible

  • is_trivially_copy_constructible

  • is_nothrow_copy_constructible

  • is_move_constructible

  • is_trivially_move_constructible

  • is_nothrow_move_constructible

  • is_assignable

  • is_trivially_assignable

  • is_nothrow_assignable

  • is_copy_assignable

  • is_trivially_copy_assignable

  • is_nothrow_copy_assignable

  • is_move_assignable

  • is_trivially_move_assignable

  • is_nothrow_move_assignable

  • is_destructible

  • is_trivially_destructible

  • is_nothrow_destructible

  • has_virtual_destructor

  • is_swappable_with

  • is_swappable

  • is_nothrow_swappable_with

  • is_nothrow_swappable

antkmsft avatar Sep 28 '22 00:09 antkmsft

BTW, I won't be surprised if some of our inheritable classes do currently have a default copy constructor and assignment, and we should =delete them to prevent slicing. The tests will tell if we do have such types.

antkmsft avatar Sep 28 '22 00:09 antkmsft

Hello, is this issue still available for assignment?

tmrna avatar May 06 '23 02:05 tmrna

Hello, is this issue still available for assignment?

As far as I know, it is.

LarryOsterman avatar May 08 '23 18:05 LarryOsterman

@tmrna, thank you for your interest! @jnyfah has just recently made a PR #4627 to test it for a single type, but there is more to cover, plus to straighten out some methods to correctly detect things like "is destructible". If you want, I think there is a way to cooperate and cover the entire SDK and all its types, and there can be plenty of work for everyone. Let me know if I can help, maybe we should split this issue into a several smaller ones that are easier to close, divide, and assign: i.e.: "cover azure.core types", cover "azure.identity types", and so on. All your contributions are appreciated, in the end we can make the issues to look right for the boundary of work that you did, it is not a problem to create as many of them as necessary, I only don't want you to do something only to find out that the very same work that was done by another contributor just days ago, as this must be disappointing, so please let me know you need my help to coordinate your efforts in that aspect; or you are free to come to that agreement without me, but I think we can make that all work.

antkmsft avatar May 15 '23 21:05 antkmsft

Resolved by @jnyfah in #4627.

I will create a subsequent issue, marked with good-first-issue tag, to cover more SDK types. People are free to take as big chunk of work as they want - we'll create a sub-issue with corresponding smaller scope, and will mention contributors in the CHANGELOG.

antkmsft avatar May 26 '23 06:05 antkmsft

Subsequent issue - #4657.

antkmsft avatar May 26 '23 06:05 antkmsft