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

[associative.reqmts.general],[unord.req.general] Fix cross-references to [container.alloc.reqmts] and [container.reqmts] #7249

Merged
merged 1 commit into from
Sep 8, 2024

Conversation

CaseyCarter
Copy link
Contributor

@CaseyCarter CaseyCarter commented Sep 6, 2024

Both paragraphs incorrectly point to [container.reqmts] instead of [container.alloc.reqmts] for "the requirements of an allocator-aware container".

Both paragraphs state "the requirements placed on value_type in [container.alloc.reqmts] apply instead to key_type and mapped_type" for map, multimap, set, and multiset. This would be nonsense, since those containers never allocate or (or allocator-construct) key_type or map_type objects: do allocator-construct value_type objects. The examples in each paragraph also make the error obvious: there are no CopyAssignable requirements in [container.alloc.reqmts].

I suspect these pairs of xrefs in each paragraph were transposed sometime in the past and we just haven't noticed.

… to [container.alloc.reqmts] and [container.reqmts]

Both paragraphs incorrectly point to [container.reqmts] instead of [container.alloc.reqmts] for "the requirements of an allocator-aware container".

Both paragraphs state "the requirements placed on `value_type` in [container.alloc.reqmts]" apply instead to `key_type` and `mapped_type`" for `map`, `multimap`, `set`, and `multiset`. This would be nonsense, since those containers never allocate or (or allocator-construct) `key_type` or `map_type` objects: _do_ allocator-construct `value_type` objects. The examples in each paragraph also make the error obvious: there are no `CopyAssignable` requirements in [container.alloc.reqmts].

I suspect these pairs of xrefs in each paragraph were transposed sometime in the past and we just haven't noticed.
@cjdb
Copy link
Contributor

cjdb commented Sep 7, 2024

We may want to defer this to LWG first: I don't see any other wording that prohibits map<K&, V> (but that has issues), and it would be strange to allow map<K&&, V>, but not any of map<K&, V>, unordered_map<K&, V>, or unordered_map<K&&, V>. It might also be worth raising whether we want to continue supporting cv-qualified key_types, especially since MapLike<K const, V> is the same as MapLike<K, V>, but with additional instantiations.

@jwakely
Copy link
Member

jwakely commented Sep 7, 2024

I suspect these pairs of xrefs in each paragraph were transposed sometime in the past and we just haven't noticed.

Yes, d09a77e fixed another case of this.

And dd32b7e, and 4fac9f9

The subclause [container.alloc.requirements] was added by 93ff092, before that they didn't have their own subclause, so referring to [container.reqmts] was correct.

@CaseyCarter
Copy link
Contributor Author

CaseyCarter commented Sep 8, 2024

We may want to defer this to LWG first: I don't see any other wording that prohibits map<K&, V> (but that has issues), and it would be strange to allow map<K&&, V>, but not any of map<K&, V>, unordered_map<K&, V>, or unordered_map<K&&, V>. It might also be worth raising whether we want to continue supporting cv-qualified key_types, especially since MapLike<K const, V> is the same as MapLike<K, V>, but with additional instantiations.

Changing "the requirements placed on value_type in [container.alloc.reqmts] apply instead to key_type and mapped_type" to "the requirements placed on value_type in [container.reqmts] apply instead to key_type and mapped_type" doesn't affect this. The first requirement on value_type in [container.reqmts] is:

typename X::value_type

-2- Result: T
-3- Preconditions: T is Cpp17Erasable from X (see [container.alloc.reqmts], below).

which still implicitly requires value_type* to be a valid type, and therefore key_type* and map_type*. This still forbids key_type and map_type to be reference types just as effectively as before. It's not terribly explicit - and like I said seems silly, since containers don't pass pointers of those types to Allocator::construct and Allocator::destroy - but swapping these cross-references doesn't change this property.

@cjdb
Copy link
Contributor

cjdb commented Sep 8, 2024

Thanks for clarifying!

@jensmaurer jensmaurer merged commit 9d9a377 into cplusplus:main Sep 8, 2024
2 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.

4 participants