<exception>: Optimize exception_ptr operations - #6403
Open
nam tran (namtran1812) wants to merge 2 commits into
Open
<exception>: Optimize exception_ptr operations#6403nam tran (namtran1812) wants to merge 2 commits into
nam tran (namtran1812) wants to merge 2 commits into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR appears to refactor exception_ptr internals by removing helper calls and directly operating on the underlying _Data* fields (constructors, swap, bool conversion, and equality).
Changes:
- Made
exception_ptrdefault/null constructors empty. - Replaced
operator=(nullptr_t)implementation with swap-based reset. - Reimplemented
swap,operator bool, andoperator==using direct member access.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+241
to
+243
| exception_ptr() noexcept {} | ||
|
|
||
| exception_ptr(nullptr_t) noexcept { | ||
| __ExceptionPtrCreate(this); | ||
| } | ||
| exception_ptr(nullptr_t) noexcept {} |
Comment on lines
264
to
268
| exception_ptr& operator=(nullptr_t) noexcept { | ||
| exception_ptr _Ptr; | ||
| __ExceptionPtrAssign(this, &_Ptr); | ||
| swap(*this, _Ptr); | ||
| return *this; | ||
| } |
|
|
||
| explicit operator bool() const noexcept { | ||
| return __ExceptionPtrToBool(this); | ||
| return _Data1 != nullptr; |
Comment on lines
+285
to
+290
| void* const _Data1 = _Lhs._Data1; | ||
| void* const _Data2 = _Lhs._Data2; | ||
| _Lhs._Data1 = _Rhs._Data1; | ||
| _Lhs._Data2 = _Rhs._Data2; | ||
| _Rhs._Data1 = _Data1; | ||
| _Rhs._Data2 = _Data2; |
|
|
||
| _NODISCARD friend bool operator==(const exception_ptr& _Lhs, const exception_ptr& _Rhs) noexcept { | ||
| return __ExceptionPtrCompare(&_Lhs, &_Rhs); | ||
| return _Lhs._Data1 == _Rhs._Data1; |
Author
|
@microsoft-github-policy-service agree |
Author
|
Constructors / null assignment: _Data1 and _Data2 have in-class {} initializers, so an empty constructor initializes both representation words to null. This is also the optimization proposed in #6387.
operator bool: _Data1 corresponds to the stored shared_ptr::_Ptr; shared_ptr::operator bool() is implemented as get() != nullptr, so this preserves the existing helper's semantics.
Equality: __ExceptionPtrCompare delegates to shared_ptr::operator==, which compares get() only rather than the control block. _Data1 corresponds to get(), so comparing _Data1 is intentionally equivalent; comparing _Data2 too would actually change semantics.
________________________________
From: Copilot ***@***.***>
Sent: Saturday, August 15, 2026 1:56 PM
To: microsoft/STL ***@***.***>
Cc: Tran, Nam H. ***@***.***>; Mention ***@***.***>
Subject: Re: [microsoft/STL] <exception>: Optimize exception_ptr operations (PR #6403)
[External Email]
@Copilot commented on this pull request.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR appears to refactor exception_ptr internals by removing helper calls and directly operating on the underlying _Data* fields (constructors, swap, bool conversion, and equality).
Changes:
* Made exception_ptr default/null constructors empty.
* Replaced operator=(nullptr_t) implementation with swap-based reset.
* Reimplemented swap, operator bool, and operator== using direct member access.
________________________________
💡 Add a code-review agent skill</microsoft/STL/new/main?filename=.github/skills/code-review/SKILL.md> or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.<https://docs.github.com/en/copilot/how-tos/use-copilot-agents/request-a-code-review/use-code-review#mcp-servers-and-agent-skills>
________________________________
In stl/inc/exception<#6403 (comment)>:
+ exception_ptr() noexcept {}
- exception_ptr(nullptr_t) noexcept {
- __ExceptionPtrCreate(this);
- }
+ exception_ptr(nullptr_t) noexcept {}
These constructors no longer call __ExceptionPtrCreate(this). Unless _Data1/_Data2 have in-class initializers (not shown in this diff), this will leave the object in an uninitialized/invalid state and will later call __ExceptionPtrDestroy(this) in the destructor on garbage values. Please restore the creation call, or explicitly establish the required invariant for an 'empty' exception_ptr (e.g., by initializing all representation fields consistently with what __ExceptionPtrCreate would produce).
________________________________
In stl/inc/exception<#6403 (comment)>:
exception_ptr& operator=(nullptr_t) noexcept {
exception_ptr _Ptr;
- __ExceptionPtrAssign(this, &_Ptr);
+ swap(*this, _Ptr);
return *this;
}
This swap-based reset assumes _Ptr is a valid 'empty' exception_ptr. With the updated empty constructors, _Ptr may be uninitialized/invalid, and swapping it into *this (and then destroying it) can lead to destroying an invalid representation. This should be changed back to using the runtime helper (__ExceptionPtrAssign) or otherwise ensure _Ptr is constructed into the correct empty representation before swapping.
________________________________
In stl/inc/exception<#6403 (comment)>:
return *this;
}
explicit operator bool() const noexcept {
- return __ExceptionPtrToBool(this);
+ return _Data1 != nullptr;
Replacing __ExceptionPtrToBool(this) with _Data1 != nullptr hard-codes assumptions about the internal representation. If emptiness depends on more than _Data1 (e.g., _Data2, tagged pointers, or sentinel values), this will return incorrect results. Consider delegating back to __ExceptionPtrToBool or implementing the same invariant check that the helper uses.
________________________________
In stl/inc/exception<#6403 (comment)>:
+ void* const _Data1 = _Lhs._Data1;
+ void* const _Data2 = _Lhs._Data2;
+ _Lhs._Data1 = _Rhs._Data1;
+ _Lhs._Data2 = _Rhs._Data2;
+ _Rhs._Data1 = _Data1;
+ _Rhs._Data2 = _Data2;
The local variables _Data1/_Data2 shadow the member names, which makes the swap implementation harder to read and easier to mis-edit. Consider renaming the locals (e.g., _LhsData1, _LhsData2) to avoid confusion.
________________________________
In stl/inc/exception<#6403 (comment)>:
}
_NODISCARD friend bool operator==(const exception_ptr& _Lhs, const exception_ptr& _Rhs) noexcept {
- return __ExceptionPtrCompare(&_Lhs, &_Rhs);
+ return _Lhs._Data1 == _Rhs._Data1;
Changing equality from __ExceptionPtrCompare(&_Lhs, &_Rhs) to comparing only _Data1 can produce incorrect equality results if _Data2 participates in identity (or if the runtime compare accounts for more than raw pointer equality). This should either compare the full representation (_Data1 and _Data2) or delegate back to the helper used previously.
—
Reply to this email directly, view it on GitHub<#6403?email_source=notifications&email_token=BF34ZSVTJR5WEAF4V2VNC4T5KCP6DA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIOJUGQZTSMBXGAYKM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#pullrequestreview-4944390700>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/BF34ZSTOP4GI7ZVOQZYLJBT5KCP6DAVCNFSNUABFKJSXA33TNF2G64TZHMZDANBVHEZTQMRVHNEXG43VMU5TKMJWGA3DCNRRGI2KC5QC>.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS<https://github.com/notifications/mobile/ios/BF34ZSQ7GUYQXGQKUWSTQHD5KCP6DA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIOJUGQZTSMBXGAYKM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y> and Android<https://github.com/notifications/mobile/android/BF34ZSQVT3YZ2MALXIZJIXT5KCP6DA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIOJUGQZTSMBXGAYKM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>. Download it today!
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
Suppressed comments (1)
stl/inc/exception:290
- This change inlines
swapby directly swapping_Data1/_Data2instead of using__ExceptionPtrSwap. That increases coupling to the current internal representation and makes future representation changes (or CRT-side invariants) more risky because some operations still rely on CRT functions while others do not. Recommendation (moderate): prefer delegating to__ExceptionPtrSwapto keep all core operations consistent with the CRT’s canonical behavior.
friend void swap(exception_ptr& _Lhs, exception_ptr& _Rhs) noexcept {
void* const _LhsData1 = _Lhs._Data1;
void* const _LhsData2 = _Lhs._Data2;
_Lhs._Data1 = _Rhs._Data1;
_Lhs._Data2 = _Rhs._Data2;
_Rhs._Data1 = _LhsData1;
_Rhs._Data2 = _LhsData2;
}
Comment on lines
+241
to
+243
| exception_ptr() noexcept {} | ||
|
|
||
| exception_ptr(nullptr_t) noexcept {} |
Comment on lines
+270
to
+272
| explicit operator bool() const noexcept { | ||
| return _Data1 != nullptr; | ||
| } |
Comment on lines
+293
to
+295
| _NODISCARD friend bool operator==(const exception_ptr& _Lhs, const exception_ptr& _Rhs) noexcept { | ||
| return _Lhs._Data1 == _Rhs._Data1; | ||
| } |
Author
|
Good point — renamed the temporaries to _LhsData1 and _LhsData2 for clarity.
________________________________
From: Copilot ***@***.***>
Sent: Saturday, August 15, 2026 1:56 PM
To: microsoft/STL ***@***.***>
Cc: Tran, Nam H. ***@***.***>; Mention ***@***.***>
Subject: Re: [microsoft/STL] <exception>: Optimize exception_ptr operations (PR #6403)
[External Email]
@Copilot commented on this pull request.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR appears to refactor exception_ptr internals by removing helper calls and directly operating on the underlying _Data* fields (constructors, swap, bool conversion, and equality).
Changes:
* Made exception_ptr default/null constructors empty.
* Replaced operator=(nullptr_t) implementation with swap-based reset.
* Reimplemented swap, operator bool, and operator== using direct member access.
________________________________
💡 Add a code-review agent skill</microsoft/STL/new/main?filename=.github/skills/code-review/SKILL.md> or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.<https://docs.github.com/en/copilot/how-tos/use-copilot-agents/request-a-code-review/use-code-review#mcp-servers-and-agent-skills>
________________________________
In stl/inc/exception<#6403 (comment)>:
+ exception_ptr() noexcept {}
- exception_ptr(nullptr_t) noexcept {
- __ExceptionPtrCreate(this);
- }
+ exception_ptr(nullptr_t) noexcept {}
These constructors no longer call __ExceptionPtrCreate(this). Unless _Data1/_Data2 have in-class initializers (not shown in this diff), this will leave the object in an uninitialized/invalid state and will later call __ExceptionPtrDestroy(this) in the destructor on garbage values. Please restore the creation call, or explicitly establish the required invariant for an 'empty' exception_ptr (e.g., by initializing all representation fields consistently with what __ExceptionPtrCreate would produce).
________________________________
In stl/inc/exception<#6403 (comment)>:
exception_ptr& operator=(nullptr_t) noexcept {
exception_ptr _Ptr;
- __ExceptionPtrAssign(this, &_Ptr);
+ swap(*this, _Ptr);
return *this;
}
This swap-based reset assumes _Ptr is a valid 'empty' exception_ptr. With the updated empty constructors, _Ptr may be uninitialized/invalid, and swapping it into *this (and then destroying it) can lead to destroying an invalid representation. This should be changed back to using the runtime helper (__ExceptionPtrAssign) or otherwise ensure _Ptr is constructed into the correct empty representation before swapping.
________________________________
In stl/inc/exception<#6403 (comment)>:
return *this;
}
explicit operator bool() const noexcept {
- return __ExceptionPtrToBool(this);
+ return _Data1 != nullptr;
Replacing __ExceptionPtrToBool(this) with _Data1 != nullptr hard-codes assumptions about the internal representation. If emptiness depends on more than _Data1 (e.g., _Data2, tagged pointers, or sentinel values), this will return incorrect results. Consider delegating back to __ExceptionPtrToBool or implementing the same invariant check that the helper uses.
________________________________
In stl/inc/exception<#6403 (comment)>:
+ void* const _Data1 = _Lhs._Data1;
+ void* const _Data2 = _Lhs._Data2;
+ _Lhs._Data1 = _Rhs._Data1;
+ _Lhs._Data2 = _Rhs._Data2;
+ _Rhs._Data1 = _Data1;
+ _Rhs._Data2 = _Data2;
The local variables _Data1/_Data2 shadow the member names, which makes the swap implementation harder to read and easier to mis-edit. Consider renaming the locals (e.g., _LhsData1, _LhsData2) to avoid confusion.
________________________________
In stl/inc/exception<#6403 (comment)>:
}
_NODISCARD friend bool operator==(const exception_ptr& _Lhs, const exception_ptr& _Rhs) noexcept {
- return __ExceptionPtrCompare(&_Lhs, &_Rhs);
+ return _Lhs._Data1 == _Rhs._Data1;
Changing equality from __ExceptionPtrCompare(&_Lhs, &_Rhs) to comparing only _Data1 can produce incorrect equality results if _Data2 participates in identity (or if the runtime compare accounts for more than raw pointer equality). This should either compare the full representation (_Data1 and _Data2) or delegate back to the helper used previously.
—
Reply to this email directly, view it on GitHub<#6403?email_source=notifications&email_token=BF34ZSVTJR5WEAF4V2VNC4T5KCP6DA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIOJUGQZTSMBXGAYKM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#pullrequestreview-4944390700>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/BF34ZSTOP4GI7ZVOQZYLJBT5KCP6DAVCNFSNUABFKJSXA33TNF2G64TZHMZDANBVHEZTQMRVHNEXG43VMU5TKMJWGA3DCNRRGI2KC5QC>.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS<https://github.com/notifications/mobile/ios/BF34ZSQ7GUYQXGQKUWSTQHD5KCP6DA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIOJUGQZTSMBXGAYKM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y> and Android<https://github.com/notifications/mobile/android/BF34ZSQVT3YZ2MALXIZJIXT5KCP6DA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIOJUGQZTSMBXGAYKM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>. Download it today!
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Author
|
The empty constructors are intentional. `_Data1` and `_Data2` both have in-class `{}` initializers, so an empty constructor still initializes the full representation to null. `__ExceptionPtrCreate` currently placement-constructs an empty `shared_ptr`, which produces the same state. This is also the optimization proposed in #6387.
This preserves the existing semantics. `__ExceptionPtrToBool` delegates to `shared_ptr::operator bool()`, which is implemented as `get() != nullptr`. `_Data1` corresponds to the stored pointer / `get()` value, so `_Data1 != nullptr` is equivalent to the helper.
`__ExceptionPtrCompare` delegates to `shared_ptr::operator==`, and `shared_ptr::operator==` compares `get()` only, not the control block. `_Data1` corresponds to `get()`, so comparing `_Data1` is intentionally equivalent. Comparing `_Data2` as well would actually change the existing equality semantics.
________________________________
From: Copilot ***@***.***>
Sent: Saturday, August 15, 2026 2:29 PM
To: microsoft/STL ***@***.***>
Cc: Tran, Nam H. ***@***.***>; Mention ***@***.***>
Subject: Re: [microsoft/STL] <exception>: Optimize exception_ptr operations (PR #6403)
[External Email]
@Copilot commented on this pull request.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
Suppressed comments (1)
stl/inc/exception:290
* This change inlines swap by directly swapping _Data1/_Data2 instead of using __ExceptionPtrSwap. That increases coupling to the current internal representation and makes future representation changes (or CRT-side invariants) more risky because some operations still rely on CRT functions while others do not. Recommendation (moderate): prefer delegating to __ExceptionPtrSwap to keep all core operations consistent with the CRT’s canonical behavior.
friend void swap(exception_ptr& _Lhs, exception_ptr& _Rhs) noexcept {
void* const _LhsData1 = _Lhs._Data1;
void* const _LhsData2 = _Lhs._Data2;
_Lhs._Data1 = _Rhs._Data1;
_Lhs._Data2 = _Rhs._Data2;
_Rhs._Data1 = _LhsData1;
_Rhs._Data2 = _LhsData2;
}
________________________________
In stl/inc/exception<#6403 (comment)>:
+ exception_ptr() noexcept {}
+
+ exception_ptr(nullptr_t) noexcept {}
The default and nullptr_t constructors no longer call __ExceptionPtrCreate(this), but the destructor still unconditionally calls __ExceptionPtrDestroy(this). This changes the object’s lifetime contract with the CRT (and risks Destroy being called on a state that wasn’t initialized via Create). Recommendation (mandatory): restore the __ExceptionPtrCreate(this) calls in both constructors, or adjust the destructor/invariant so that __ExceptionPtrDestroy is only invoked on states created/assigned via the CRT APIs.
________________________________
In stl/inc/exception<#6403 (comment)>:
+ explicit operator bool() const noexcept {
+ return _Data1 != nullptr;
+ }
exception_ptr truthiness and equality previously delegated to CRT helpers (__ExceptionPtrToBool, __ExceptionPtrCompare). Replacing these with _Data1 checks/comparisons can break correctness if the representation/semantics depend on both fields (or on non-trivial comparison logic), and it can break ABI/behavior expectations across toolset/CRT versions. Recommendation (mandatory): use the CRT-provided __ExceptionPtrToBool and __ExceptionPtrCompare (or an equivalently correct comparison that accounts for the full representation).
________________________________
In stl/inc/exception<#6403 (comment)>:
+ _NODISCARD friend bool operator==(const exception_ptr& _Lhs, const exception_ptr& _Rhs) noexcept {
+ return _Lhs._Data1 == _Rhs._Data1;
+ }
exception_ptr truthiness and equality previously delegated to CRT helpers (__ExceptionPtrToBool, __ExceptionPtrCompare). Replacing these with _Data1 checks/comparisons can break correctness if the representation/semantics depend on both fields (or on non-trivial comparison logic), and it can break ABI/behavior expectations across toolset/CRT versions. Recommendation (mandatory): use the CRT-provided __ExceptionPtrToBool and __ExceptionPtrCompare (or an equivalently correct comparison that accounts for the full representation).
—
Reply to this email directly, view it on GitHub<#6403?email_source=notifications&email_token=BF34ZSUIN2R5KPNSBKLCSSD5KCTW7A5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIOJUGQ2DINRQGM2KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#pullrequestreview-4944446034>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/BF34ZSQOBIWKVHBA2VUXYKT5KCTW7AVCNFSNUABFKJSXA33TNF2G64TZHMZDANBVHEZTQMRVHNEXG43VMU5TKMJWGA3DCNRRGI2KC5QC>.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS<https://github.com/notifications/mobile/ios/BF34ZSSBWYNC77AXGEVKDWL5KCTW7A5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIOJUGQ2DINRQGM2KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y> and Android<https://github.com/notifications/mobile/android/BF34ZSQ22U6VTAI6ZZNZRD35KCTW7A5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIOJUGQ2DINRQGM2KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>. Download it today!
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Optimizes several
std::exception_ptroperations that currently call out-of-line CRT helpers even though the representation is already known in<exception>.This change:
nullptr_tconstructor rely on the existing null member initializersoperator bool()as_Data1 != nullptr_Data1directly inoperator==_Data1and_Data2directlynullptr_tusing swap-with-empty, avoiding__ExceptionPtrAssignThe exported CRT helper functions remain unchanged for ABI compatibility.
Fixes #6387.
Rationale
exception_ptris intentionally laid out to matchshared_ptr<const _EXCEPTION_RECORD>. Its two data members correspond to the shared pointer's stored pointer and control block, and the implementation already contains astatic_assertenforcing matching size and alignment.The removed out-of-line calls currently perform operations equivalent to the inlined implementations above:
__ExceptionPtrCreatedefault-constructs an emptyshared_ptr__ExceptionPtrToBoolconverts the underlyingshared_ptrtobool__ExceptionPtrComparecompares the underlyingshared_ptr__ExceptionPtrSwapswaps the underlyingshared_ptrFor
operator=(nullptr_t), swapping with an emptyexception_ptrpreserves ownership semantics and allows the temporary's destructor to release the previous control block correctly.Validation
The existing
Dev11_0299014_exception_ptr_requirementstest already exercises the affected semantics, including:nullptrconstructionnullptrNo ABI-visible layout or exported CRT symbol is changed.