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

Test each type for having the expected default operations #3964

Closed
antkmsft opened this issue Sep 28, 2022 · 6 comments
Closed

Test each type for having the expected default operations #3964

antkmsft opened this issue Sep 28, 2022 · 6 comments
Labels
EngSys This issue is impacting the engineering system. good first issue This issue tracks work that may be a good starting point for a first-time contributor test-enhancement

Comments

@antkmsft
Copy link
Member

antkmsft commented Sep 28, 2022

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

@ghost ghost added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Sep 28, 2022
@antkmsft
Copy link
Member Author

antkmsft commented Sep 28, 2022

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.

@RickWinter RickWinter added good first issue This issue tracks work that may be a good starting point for a first-time contributor EngSys This issue is impacting the engineering system. test-enhancement labels Oct 4, 2022
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Oct 4, 2022
@RickWinter RickWinter assigned RickWinter and antkmsft and unassigned RickWinter Oct 4, 2022
@tmrna
Copy link

tmrna commented May 6, 2023

Hello, is this issue still available for assignment?

@LarryOsterman
Copy link
Member

Hello, is this issue still available for assignment?

As far as I know, it is.

@antkmsft
Copy link
Member Author

@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
Copy link
Member Author

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
Copy link
Member Author

Subsequent issue - #4657.

azure-sdk added a commit to azure-sdk/vcpkg that referenced this issue Jun 2, 2023
## 1.10.0 (2023-06-01)

### Features Added

- Added `Azure::Core::Uuid::AsArray()` and `Azure::Core::Uuid::CreateFromArray()` to enable reading or writing from an existing UUID. This is useful when the UUID was generated outside the Azure SDK, or needs to be used from a component outside the Azure SDK.

### Other Changes

- [[microsoft#3964]](Azure/azure-sdk-for-cpp#3964) Ensuring some Azure SDK types have the expected default operations. (A community contribution, courtesy of _[jnyfah](https://github.com/jnyfah)_)

### Acknowledgments

Thank you to our developer community members who helped to make Azure Core better with their contributions to this release:

- Jennifer Chukwu _([GitHub](https://github.com/jnyfah))_
dan-shaw pushed a commit to microsoft/vcpkg that referenced this issue Jun 5, 2023
## 1.10.0 (2023-06-01)

### Features Added

- Added `Azure::Core::Uuid::AsArray()` and `Azure::Core::Uuid::CreateFromArray()` to enable reading or writing from an existing UUID. This is useful when the UUID was generated outside the Azure SDK, or needs to be used from a component outside the Azure SDK.

### Other Changes

- [[#3964]](Azure/azure-sdk-for-cpp#3964) Ensuring some Azure SDK types have the expected default operations. (A community contribution, courtesy of _[jnyfah](https://github.com/jnyfah)_)

### Acknowledgments

Thank you to our developer community members who helped to make Azure Core better with their contributions to this release:

- Jennifer Chukwu _([GitHub](https://github.com/jnyfah))_
@github-actions github-actions bot locked and limited conversation to collaborators Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
EngSys This issue is impacting the engineering system. good first issue This issue tracks work that may be a good starting point for a first-time contributor test-enhancement
Projects
None yet
Development

No branches or pull requests

4 participants