Skip to content
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

alternative == method linkage fix #140

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

alternative == method linkage fix #140

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
21 changes: 19 additions & 2 deletions Sources/System/FilePath/FilePathComponents.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,16 @@ extension FilePath {
}
}

extension FilePath.Component {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
extension FilePath.Component {
extension FilePath.Component: Hashable {

@lorentey would this be possible to do (without availability) as the ABI-stable build picked up the symbols?

Also, would we still need the public func below if we added the conformance explicitly?

Copy link
Member Author

Choose a reason for hiding this comment

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

awaiting @lorentey input

Copy link
Member

@lorentey lorentey Aug 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The sinking of the Equatable/Hashable conformances from _StrSlice to Component is probably fine, ABI-wise. If any other public type conformed to _StrSlice, then it'll need to do the same.

_StrSlice will need to stop conforming to Equatable/Hashable.

public static func == (lhs: Self, rhs: Self) -> Bool {
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, this would be a newly exported symbol, so it needs to come with an availability declaration.

To clean up the mess that resulted from adding public members to an internal type, we should add this implementation in a form that is back-deployable.

return Self.strSliceEqual(lhs: lhs, rhs: rhs)
}

public func hash(into hasher: inout Hasher) {
Copy link
Member

Choose a reason for hiding this comment

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

Same problem here -- this is a new symbol, so it needs to either have an @available attribute combined with @backDeployed(before:), or it needs to be declared @_alwaysEmitIntoClient.

return self.strSliceHash(into: &hasher)
}
}

@available(/*System 0.0.2: macOS 12.0, iOS 15.0, watchOS 8.0, tvOS 15.0*/iOS 8, *)
extension FilePath.Component {

Expand Down Expand Up @@ -166,15 +176,22 @@ extension _StrSlice {
internal var _systemString: SystemString { SystemString(_slice) }
}
extension _StrSlice {
public static func == (lhs: Self, rhs: Self) -> Bool {
public static func strSliceEqual(lhs: Self, rhs: Self) -> Bool {
Copy link
Member

Choose a reason for hiding this comment

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

If we want to go this way, let's at least put an end to adding any new public members of internal types. That only leads to sadness.

Suggested change
public static func strSliceEqual(lhs: Self, rhs: Self) -> Bool {
internal static func strSliceEqual(lhs: Self, rhs: Self) -> Bool {

lhs._slice.elementsEqual(rhs._slice)
}
public func hash(into hasher: inout Hasher) {
public static func == (lhs: Self, rhs: Self) -> Bool {
Copy link
Member

Choose a reason for hiding this comment

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

We should remove this function altogether. (After verifying that it doesn't end up getting exported somehow in the ABI stable binary.)

return Self.strSliceEqual(lhs: lhs, rhs: rhs)
}

public func strSliceHash(into hasher: inout Hasher) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public func strSliceHash(into hasher: inout Hasher) {
internal func strSliceHash(into hasher: inout Hasher) {

hasher.combine(_slice.count) // discriminator
for element in _slice {
hasher.combine(element)
}
}
public func hash(into hasher: inout Hasher) {
Copy link
Member

Choose a reason for hiding this comment

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

Same here -- this function should not exist.

return self.strSliceHash(into: &hasher)
}
}
internal protocol _PathSlice: _StrSlice {
var _path: FilePath { get }
Expand Down