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

<vector>: assign_range() is missing Mandates #5076

Open
hewillk opened this issue Nov 11, 2024 · 7 comments · May be fixed by #5086
Open

<vector>: assign_range() is missing Mandates #5076

hewillk opened this issue Nov 11, 2024 · 7 comments · May be fixed by #5086
Labels
bug Something isn't working ranges C++20/23 ranges

Comments

@hewillk
Copy link
Contributor

hewillk commented Nov 11, 2024

According to [sequence.reqmts], a.assign_range(rg) has the Mandates of assignable_from<T&, ranges​::​range_reference_t<R>> s modeled, however, this is missed in the MSVC-STL implementation.

The following should be rejected:

#include <vector>
 
struct Int {
  void operator=(int);
  Int(int);
};

int main() {
  std::vector<Int> v;
  v.assign_range(std::vector{42}); // only libstdc++ rejects
}

https://godbolt.org/z/TrGsbcndj

Not sure if other containers have similar issues.

@CaseyCarter CaseyCarter added the bug Something isn't working label Nov 11, 2024
@CaseyCarter
Copy link
Member

I'm sure I missed enforcing this requirement on the type and value category of the assignment expression for all of the assign_range implementations, we should audit.

@frederick-vs-ja
Copy link
Contributor

I believe our assignment expressions are fine:

*_Next = *_First;

*_Mid = *_First;

*_Myfirst = *_First;

STL/stl/inc/list

Line 1331 in 25dc2b7

_Old->_Myval = *_First;

_Next->_Myval = *_UFirst;

For vector<bool, A>, we're using copy-initialization instead of assignment. I think this is still conforming - but if I'm wrong, we need to swich to use bool _Tmp; _Tmp = *_First;.

const bool _Tmp = *_First;

const bool _Tmp = *_First;


Note that std::is_assignable_v<Int&, std::ranges::range_reference_t<std::vector<int>&>> is true for your example. So I guess this is arguably a defect in the standard that it's overly strict to use assignable_from in Mandates of assign_range functions. Perhaps it would be OK to just reuse the well-formedness of declval<T&>() = *ranges::begin(rg).
(is_assignable_v can be overly strict when the iterator of rg dereferences to a prvalue, the element type of rg is not movable, and the operator= takes that element type by value.)

@hewillk
Copy link
Contributor Author

hewillk commented Nov 12, 2024

For vector<bool, A>, we're using copy-initialization instead of assignment. I think this is still conforming - but if I'm wrong, we need to swich to use bool _Tmp; _Tmp = *_First;.

STL/stl/inc/vector

const bool _Tmp = *_First;

_Container_compatible_range<bool> guaranteed this is ok, right?

@frederick-vs-ja
Copy link
Contributor

Also, common_reference_with in assignable_from doesn't seem correct to me. I found that it was originally added in ericniebler/stl2#150 (although significantly modified in C++20).

I think when we use assignable_from to constrain something, it just corresponds to some assignment expressions where common_reference_t is unnecessary.

@StephanTLavavej StephanTLavavej changed the title <vector>: assign_range() is missing __Mandates__ <vector>: assign_range() is missing Mandates Nov 13, 2024
@StephanTLavavej
Copy link
Member

We talked about this at the weekly maintainer meeting and we think that adding a bunch of static_asserts to enforce these Mandates is quick and easy, with the disclaimer that such static_asserts imply nothing about the moral worth of the Standard's yucky requirements 🤮 (which @CaseyCarter wishes he could take back - I am omitting an entire glorious rant about the assignable_from concept here 😹).

@StephanTLavavej StephanTLavavej added the ranges C++20/23 ranges label Nov 15, 2024
@hewillk
Copy link
Contributor Author

hewillk commented Nov 16, 2024

Note that string::assign_range doesn't have Mandates for this, not sure if this deserves an LWG.

@CaseyCarter
Copy link
Member

Note that string::assign_range doesn't have Mandates for this, not sure if this deserves an LWG.

Speaking for only myself, I'm more likely to submit an LWG issue to remove the requirement elsewhere than to submit an issue to add it to basic_string.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ranges C++20/23 ranges
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants