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

Added to_array #14

Merged
merged 18 commits into from
Oct 4, 2024
Merged

Added to_array #14

merged 18 commits into from
Oct 4, 2024

Conversation

anarthal
Copy link
Contributor

@anarthal anarthal commented Oct 3, 2024

No description provided.

@pdimov
Copy link
Member

pdimov commented Oct 3, 2024

  1. the static asserts don't actually test what the code requires. The array elements are of type remove_cv_t<T>, not T, so the conditions being asserted should be is_constructible<remove_cv_t<T>, T&> and is_constructible<remove_cv_t<T>, T&&> respectively.
  2. documentation
  3. the tests should test const input elements, e.g. int const, std::vector<int> const.
  4. the constexpr tests should still check that the result is correct (at runtime).

@anarthal
Copy link
Contributor Author

anarthal commented Oct 3, 2024

Regarding 1. cppreference it states it as I wrote it - is it a defect in cppreference?

@pdimov
Copy link
Member

pdimov commented Oct 3, 2024

cppreference correctly reflects what the standard says in https://eel.is/c++draft/array.creation, but I think that this is a defect in the standard. I'll ask.

@pdimov
Copy link
Member

pdimov commented Oct 3, 2024

Please add lvalue tests for const int and l/rvalue tests for const.

Also, let's use the correct is_constructible<remove_cv_t<T>, T&> in both code and docs, regardless of what the standard says.

@pdimov
Copy link
Member

pdimov commented Oct 3, 2024

No, sorry. The array elements being constructed are of type remove_cv_t<T>. The arguments are of type T& or T&& respectively. Ergo, the conditions are is_constructible<remove_cv_t<T>, T&> and is_constructible<remove_cv_t<T>, T&&>, respectively.

@anarthal
Copy link
Contributor Author

anarthal commented Oct 4, 2024

Long day yesterday. Should be fixed now. If I've understood this correctly, using remove_cvref_t doesn't make sense because C arrays can't contain references, and thus remove_cv_t is more accurate.

I've left the multi-dimensional checking condition as is_array<T> because is_array<const/volatile int[N]> is also true.

@pdimov pdimov merged commit d2d5cbe into boostorg:develop Oct 4, 2024
58 of 59 checks passed
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.

2 participants