-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[cxx-interop] Add CxxStack
protocol for std::stack
ergonomics.
#81087
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
base: main
Are you sure you want to change the base?
Conversation
376996a
to
7fef830
Compare
@swift-ci please test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution @CrazyFanFan ! I left some preliminary comments for now
return s; | ||
} | ||
|
||
StackOfInt initEmtypeStackOfCInt() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
StackOfInt initEmtypeStackOfCInt() { | |
StackOfInt initEmptyStackOfCInt() { |
// | ||
//===----------------------------------------------------------------------===// | ||
|
||
public protocol CxxStack<Element> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This protocol should be documented
@inlinable var isEmpty: Bool { empty() } | ||
|
||
@inlinable | ||
func top() -> Element { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not return an optional value instead of asserting this pre-condition here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback!
- In C++, accessing the top of an empty stack is undefined behavior, so a similar effect is retained here.
- Considering bridging to Swift, using an optional value is indeed a good option.
I've been torn between these two approaches.
func __topUnsafe() -> RawReference | ||
|
||
mutating func push(_ element: Element) | ||
mutating func pop() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The behavior of calling pop()
(which calls pop_back()
on the underlying container) is undefined for empty()
stacks. I'm wondering then if that warrants either (1) this method being marked unsafe, and/or (2) wrapping this method with something like:
/// Return value indicates whether an element was successfully popped
mutating func safePop() -> Bool {
if empty() {
return false
}
__unsafePop()
return true
}
(with a better name than safePop()
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Oh, thanks for the reminder. It's more reasonable to mark this method as
unsafe
. - I think
safePop
is concise and clear enough. I'll enhance its implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Someone more familiar with Swift than me would have to comment on what the "swifty" way would be, but if we're providing a safe wrapper anyways, do we perhaps want to return Element?
containing the removed value, rather than just a boolean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I've added a mutating func safePop() -> Element?
. Currently, some tests are failing, and I'm in the process of debugging.
@@ -865,6 +865,45 @@ void swift::conformToCxxSequenceIfNeeded( | |||
} | |||
} | |||
|
|||
void swift::conformToCxxStackIfNeeded( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good with some negative test cases for inferring conformance, just to make sure it gets handled properly. In particular I'm interested in how element types that are ~Escapable
and/or ~Copyable
are handled. Do they get a diagnostic, or is the conformance just omitted? Also, can std::stack
be declared for forward declared types in C++? If so, it'd be good to have a test case for that too.
This adds a protocol to the C++ standard library overlay that will improve the ergonomics of
std::stack
when used from Swift code.As of now,
CxxStack
adds atop() -> Element
function to C++ stacks.