Skip to content

Conversation

@CaseyCarter
Copy link
Contributor

Drive-by cleanup:

  • explicitly compare pointers with nullptr in preference to if (ptr) and if (!ptr)
  • _STL_INTERNAL_CHECK checkable preconditions
  • Use constinit when available to verify constexpr constructor

Fixes #1309.

Drive-by cleanup:
* explicitly compare pointers  with `nullptr` in preference to `if (ptr)` and `if (!ptr)`
* `_STL_INTERNAL_CHECK` checkable preconditions
* Use `constinit` when available to verify `constexpr` constructor

Fixes microsoft#1309.
@CaseyCarter CaseyCarter added the bug Something isn't working label Sep 20, 2020
@CaseyCarter CaseyCarter marked this pull request as ready for review September 21, 2020 04:04
@CaseyCarter CaseyCarter requested a review from a team as a code owner September 21, 2020 04:04
@BillyONeal
Copy link
Member

construct isn't supposed to require that the allocator be rebound first; this introduces an extra rebind to support a case the standard doesn't require.

This is also inconsistent with what we do everywhere else in the node-based containers, for example

allocator_traits<_Alnode>::destroy(_Al, _STD addressof(_Ptr->_Myval));

@CaseyCarter
Copy link
Contributor Author

construct isn't supposed to require that the allocator be rebound first; this introduces an extra rebind to support a case the standard doesn't require.

On the contrary; this introduces an extra rebind to support what the Standard explicitly requires in [container.node.cons]/3.1 and [container.node.dtor]/1.

@BillyONeal
Copy link
Member

I see, so the standard does say that the type needs to match. We make no attempt to do that right now; all construct and destroy calls should probably be audited for this in the node based containers.

@CaseyCarter
Copy link
Contributor Author

I see, so the standard does say that the type needs to match. We make no attempt to do that right now; all construct and destroy calls should probably be audited for this in the node based containers.

The quoted paragraphs have nothing to do with nodes stored in node-based containers, they only describe the behavior of node-handle while it holds a node. No, I don't know why the Standard wants these two cases to have different behavior - it seems completely pointless to me. I assume it was the simplest way to specify node-handle.

@BillyONeal
Copy link
Member

The quoted paragraphs have nothing to do with nodes stored in node-based containers, they only describe the behavior of node-handle while it holds a node. No, I don't know why the Standard wants these two cases to have different behavior - it seems completely pointless to me. I assume it was the simplest way to specify node-handle.

I view it as a serious problem if construct and destroy do not match the ones the container used.

@CaseyCarter
Copy link
Contributor Author

I view it as a serious problem if construct and destroy do not match the ones the container used.

IMO you're asking for trouble If construct and destroy don't have the same behavior within a rebind family.

@BillyONeal
Copy link
Member

IMO you're asking for trouble If construct and destroy don't have the same behavior within a rebind family.

I mean, I agree, but that argues to not do the rebind.

@StephanTLavavej StephanTLavavej self-assigned this Sep 23, 2020
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

Looks good to me - I found a couple of potential clarity improvements in the test. I'll verify whether they actually compile; if so, I'll push changes.

Changes two pre-existing occurrences of double-braced sets.
@CaseyCarter CaseyCarter self-assigned this Oct 8, 2020
@CaseyCarter CaseyCarter merged commit 20a19e4 into microsoft:master Oct 9, 2020
@CaseyCarter CaseyCarter deleted the gh1309 branch October 9, 2020 01:01
@CaseyCarter
Copy link
Contributor Author

🦶 : bug smashed.
🐞

@CaseyCarter CaseyCarter removed their assignment Oct 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

<xnode_handle.h>: values should be destroyed with value_type allocator

4 participants