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.

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

Conversation

weissi
Copy link
Member

@weissi weissi commented Aug 7, 2023

alternative (to #139) fix for #138

@weissi
Copy link
Member Author

weissi commented Aug 7, 2023

@swift-ci please test

@@ -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

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.

@@ -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 {

@@ -72,6 +72,16 @@ extension FilePath {
}
}

extension FilePath.Component {
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.

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.)

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.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) {

@lorentey
Copy link
Member

I'll see if I can add a commit to this PR to make it work in all configurations of System later today!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants