<exception>: Optimize exception_ptr operations - #6403
<exception>: Optimize exception_ptr operations#6403nam 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. |
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.
| exception_ptr() noexcept {} | ||
|
|
||
| exception_ptr(nullptr_t) noexcept { | ||
| __ExceptionPtrCreate(this); | ||
| } | ||
| exception_ptr(nullptr_t) noexcept {} |
| 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; |
| 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; |
|
@microsoft-github-policy-service agree |
|
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: ***@***.***>
|
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;
}
| exception_ptr() noexcept {} | ||
|
|
||
| exception_ptr(nullptr_t) noexcept {} |
There was a problem hiding this comment.
This is incorrect. __ExceptionPtrCreate just set two internal pointers to null. Please read stl/src/excptptr.cpp carefully before making such comment.
| explicit operator bool() const noexcept { | ||
| return _Data1 != nullptr; | ||
| } |
| _NODISCARD friend bool operator==(const exception_ptr& _Lhs, const exception_ptr& _Rhs) noexcept { | ||
| return _Lhs._Data1 == _Rhs._Data1; | ||
| } |
|
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: ***@***.***>
|
|
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: ***@***.***>
|
|
Oh a vietnamese guy |
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.