Skip to content

Optimize contains method implementation in CxxSet #85488

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 20, 2025
+15 −20
Merged

Optimize contains method implementation in CxxSet #85488

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions lib/ClangImporter/ClangDerivedConformances.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -996,11 +996,6 @@ void swift::conformToCxxSetIfNeeded(ClangImporter::Implementation &impl,
insert->getResultInterfaceType());
impl.addSynthesizedProtocolAttrs(decl, {KnownProtocolKind::CxxSet});

// If this isn't a std::multiset, try to also synthesize the conformance to
// CxxUniqueSet.
if (!isStdDecl(clangDecl, {"set", "unordered_set"}))
return;

ProtocolDecl *cxxInputIteratorProto =
ctx.getProtocol(KnownProtocolKind::UnsafeCxxInputIterator);
if (!cxxInputIteratorProto)
Expand All @@ -1025,6 +1020,12 @@ void swift::conformToCxxSetIfNeeded(ClangImporter::Implementation &impl,
rawIteratorTy);
impl.addSynthesizedTypealias(decl, ctx.getIdentifier("RawMutableIterator"),
rawMutableIteratorTy);

// If this isn't a std::multiset, try to also synthesize the conformance to
// CxxUniqueSet.
if (!isStdDecl(clangDecl, {"set", "unordered_set"}))
return;

impl.addSynthesizedProtocolAttrs(decl, {KnownProtocolKind::CxxUniqueSet});
}

Expand Down
24 changes: 9 additions & 15 deletions stdlib/public/Cxx/CxxSet.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,22 @@
public protocol CxxSet<Element>: ExpressibleByArrayLiteral {
associatedtype Element
associatedtype Size: BinaryInteger
associatedtype RawIterator: UnsafeCxxInputIterator
where RawIterator.Pointee == Element
associatedtype RawMutableIterator: UnsafeCxxInputIterator
Copy link
Member

@hnrklssn hnrklssn Nov 14, 2025

Choose a reason for hiding this comment

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

Should this not be UnsafeCxxMutableInputIterator?

Copy link
Contributor Author

@CrazyFanFan CrazyFanFan Nov 14, 2025

Choose a reason for hiding this comment

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

std::set iterators are indeed immutable since modifying set elements would break the internal ordering. This aligns with the existing StdSet implementation in Swift's ClangImporter

where RawMutableIterator.Pointee == Element

// std::pair<iterator, bool> for std::set and std::unordered_set
// iterator for std::multiset
associatedtype InsertionResult
associatedtype InsertionResult

init()

func __endUnsafe() -> RawIterator
func __findUnsafe(_ value: Element) -> RawIterator
Copy link
Member

@hnrklssn hnrklssn Nov 14, 2025

Choose a reason for hiding this comment

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

This was mutating previously, is there a reason why it's not anymore?

Copy link
Contributor Author

@CrazyFanFan CrazyFanFan Nov 14, 2025

Choose a reason for hiding this comment

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

Thanks for pointing this out!

I removed mutating because neither __endUnsafe() nor __findUnsafe(_:) modifies the underlying storage—this aligns with their semantic purpose, and they’re used in the non-mutating contains(_:).

That said, I recognize this is a breaking change to the public API. Though, since these methods are prefixed with __, I wondered if that might make the change more acceptable.

I’d like to align with the project’s preferences here—would you recommend restoring the original signatures to preserve compatibility, or keeping the semantically accurate version?

Happy to implement whichever approach makes the most sense.

Copy link
Contributor

@egorzhdan egorzhdan Nov 14, 2025

Choose a reason for hiding this comment

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

The Cxx and CxxStdlib overlays aren't part of the Swift's ABI stability guarantees, so it's totally fine to change their interfaces. For this reason these two libraries are distributed as part of the toolchain, not part of the SDK, and are version-locked to the compiler.

As long as making these functions non-mutating doesn't break any tests, it should be OK to make this change.


@discardableResult
mutating func __insertUnsafe(_ element: Element) -> InsertionResult

func count(_ element: Element) -> Size
}

Expand Down Expand Up @@ -56,7 +62,7 @@ extension CxxSet {
/// in the set.
@inlinable
public func contains(_ element: Element) -> Bool {
return count(element) > 0
return self.__findUnsafe(element) != self.__endUnsafe()
}
}

Expand All @@ -65,23 +71,11 @@ extension CxxSet {
/// C++ standard library types such as `std::set` and `std::unordered_set`
/// conform to this protocol.
public protocol CxxUniqueSet<Element>: CxxSet {
override associatedtype Element
override associatedtype Size: BinaryInteger
associatedtype RawIterator: UnsafeCxxInputIterator
where RawIterator.Pointee == Element
associatedtype RawMutableIterator: UnsafeCxxInputIterator
where RawMutableIterator.Pointee == Element
override associatedtype InsertionResult
where InsertionResult: CxxPair<RawMutableIterator, Bool>

@discardableResult
mutating func __findUnsafe(_ value: Element) -> RawIterator

@discardableResult
mutating func __eraseUnsafe(_ iter: RawIterator) -> RawMutableIterator

@discardableResult
mutating func __endUnsafe() -> RawIterator
}

extension CxxUniqueSet {
Expand Down