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

static_assert that container and allocator type parameters are object types #2436

Merged
merged 9 commits into from Apr 14, 2023

Conversation

cristeigabriel
Copy link
Contributor

@cristeigabriel cristeigabriel commented Dec 19, 2021

Fixes #2408.

This PR implements a message (or more, depending on structure) when you try to instantiate an object which has inherent contradicting type requirements (ex. "pointer to reference").

I tried not to be pedantic with the implementation, but I might've went wrong with that, thus I'm sorry if I did.

Notes: I have noticed different order for implementing static asserts in containers, sometimes it's before required types and right after, if there is, friend classes, and sometimes, it's after required types and in different access modifiers (not like it changes anything, just a nitpick).

cristeigabriel and others added 5 commits December 12, 2021 19:08
…nd allocator

Fixes microsoft#2408.

This PR implements a message (or more, depending on structure)  when you try to instantiate an object which has inherent contradicting type requirements (ex. "pointer to reference").

I tried not to be pedantic with the implementation, but I might've went wrong with that, thus I'm sorry if I did.

Notes: I have noticed different order for implementing static asserts in containers, sometimes it's before required types and right after, if there is, friend classes, and sometimes, it's after required types and in different access modifiers (not like it changes anything, just a nitpick).
stl/inc/vector Outdated Show resolved Hide resolved
stl/inc/xmemory Show resolved Hide resolved
stl/inc/array Outdated Show resolved Hide resolved
Copy link
Contributor

@sam20908 sam20908 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one question:

@mnatsuhara mnatsuhara added this to Initial Review in Code Reviews Dec 28, 2021
@CaseyCarter CaseyCarter added the enhancement Something can be improved label Dec 29, 2021
@strega-nil-ms strega-nil-ms changed the title Implement compile-time restraints for reference types in containers a… static_assert that container and allocator type parameters are not reference types Jul 22, 2022
@strega-nil-ms strega-nil-ms self-assigned this Aug 24, 2022
@StephanTLavavej StephanTLavavej added the decision needed We need to choose something before working on this label Aug 24, 2022
@strega-nil-ms strega-nil-ms removed their assignment Aug 29, 2022
@StephanTLavavej StephanTLavavej removed the decision needed We need to choose something before working on this label Mar 8, 2023
@StephanTLavavej StephanTLavavej self-assigned this Mar 8, 2023
@frederick-vs-ja
Copy link
Contributor

Should we just static_assert that an element type must be an object type instead of that it must not be a reference type? IMO we don't need to prepare different error messages for cv void, function types, and reference types.

@StephanTLavavej
Copy link
Member

StephanTLavavej commented Apr 10, 2023

Yeah, that's a good idea, modulo allocator<void> being allowed. (WG21-N4944 [basic.types.general]/8 "An object type is a (possibly cv-qualified) type that is not a function type, not a reference type, and not cv void.")

I'll review and revise this PR that we've neglected for far too long (sorry! 🙀).

Updated title accordingly (yes, the title glosses over allocator<void>).

* Changed `unordered_multimap` which was originally overlooked
* Varied the wording for container adaptors
* For `map`, `multimap`, `unordered_map`, and `unordered_multimap`, checked both the keys and values
* Added `!is_function_v<_Ty>` to `allocator`
* Changed the checks outside `allocator` to `is_object_v<_Ty>` with matching messages
@StephanTLavavej StephanTLavavej changed the title static_assert that container and allocator type parameters are not reference types static_assert that container and allocator type parameters are object types Apr 10, 2023
@StephanTLavavej
Copy link
Member

Pushed:

  • Conflict-free merge with main
  • Changed unordered_multimap which was originally overlooked
  • Varied the wording for container adaptors
  • For map, multimap, unordered_map, and unordered_multimap, checked both the keys and values
  • Added !is_function_v<_Ty> to allocator
  • Changed the checks outside allocator to is_object_v<_Ty> with matching messages

@StephanTLavavej StephanTLavavej removed their assignment Apr 10, 2023
@StephanTLavavej StephanTLavavej moved this from Initial Review to Final Review in Code Reviews Apr 10, 2023
@StephanTLavavej
Copy link
Member

Relevant newly filed LWG issues:

  • LWG-3916 allocator, polymorphic_allocator, and containers should forbid cv-qualified types
  • LWG-3917 Validity of allocator<void> and possibly polymorphic_allocator<void> should be clarified

@strega-nil-ms strega-nil-ms moved this from Final Review to Ready To Merge in Code Reviews Apr 11, 2023
@StephanTLavavej StephanTLavavej self-assigned this Apr 12, 2023
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 297c123 into microsoft:main Apr 14, 2023
35 checks passed
Code Reviews automation moved this from Ready To Merge to Done Apr 14, 2023
@StephanTLavavej
Copy link
Member

Thanks for improving these diagnostics - and my apologies again for taking over a year to merge this! 🐌 🦥 🐢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Implementing is_reference_v restraints?
7 participants