Skip to content

Remove use of NSString.replacingOccurrences(of:with:) #6650

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

Jump to bottom
Merged
merged 1 commit into from
Mar 4, 2025
Merged

Remove use of NSString.replacingOccurrences(of:with:) #6650

merged 1 commit into from
Mar 4, 2025

Conversation

compnerd
Copy link
Member

Prefer to use String.replacing(_:with:) instead of the NSString operations for the strings. This should avoid an unnecessary use of Foundation API and a dispatch to the NSString API.

@compnerd
Copy link
Member Author

@swift-ci please test

@compnerd
Copy link
Member Author

@swift-ci please smoke test

MaxDesiatov
MaxDesiatov approved these changes Jun 14, 2023
View reviewed changes
Copy link
Contributor

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

Nice 👍

@MaxDesiatov
Copy link
Contributor 

NativePathExtensions.swift:21:29: error: 'replacing(_:with:maxReplacements:)' is only available in macOS 13.0 or newer
                return repr.replacing("\\", with: "\\\\")
                            ^
/Users/ec2-user/jenkins/workspace/swift-package-manager-with-xcode-self-hosted-PR-osx/branch-main/swiftpm/Sources/Basics/NativePathExtensions.swift:21:29: note: add 'if #available' version check

Not sure if it makes more sense to add #if os(macOS) checks here instead of runtime version checks. I'd expect file path functions to be frequently invoked, so a runtime check there would be undesirable.

@tomerd
Copy link
Contributor

tomerd commented Jun 14, 2023

nice, thanks!

@tomerd tomerd enabled auto-merge (squash) June 14, 2023 18:22
@compnerd
Copy link
Member Author

@MaxDesiatov I have the same concern about the frequency of this, and a call to __isOSVersionAtLeast on each iteration seems ... unnecessarily expensive. I think that #if os(macOS) is the best option here :-(

@compnerd
Copy link
Member Author

@MaxDesiatov is there a good way to annotate the availability? Ideally we could use the same path on macOS as well 😞

@neonichu
Copy link
Contributor

Since this is for main, we could consider increasing the the deployment target so that we don't need any checks.

@compnerd
Copy link
Member Author

@neonichu oh? That really would be my preference.

@MaxDesiatov
Copy link
Contributor

@swift-ci smoke test

@MaxDesiatov
Copy link
Contributor

@swift-ci test windows

@compnerd
Copy link
Member Author

@swift-ci please test Windows platform

@neonichu
Copy link
Contributor

Actually, I think the CI is using macOS 12, so that would have to be updated before we can upgrade the deployment target :/

@tomerd
Copy link
Contributor

tomerd commented Jun 20, 2023

@compnerd maybe add availability check, then we can deprecate the old path when we can raise the min target

@tomerd
Copy link
Contributor

tomerd commented Jul 11, 2023

@compnerd do you want to take this forward, or close?

@compnerd
Copy link
Member Author

I think that if we can get this sorted out, it is better. IMO, we should lean on the standard library wherever feasible.

@tomerd
Copy link
Contributor

tomerd commented Jul 11, 2023

agreed - I think that can be achieved by changing the package min deployment platform back to 12 and adding inline availability checks until we can raise the min deployment target

Kyle-Ye
Kyle-Ye reviewed Jul 12, 2023
View reviewed changes
Package.swift Outdated
@@ -69,7 +69,7 @@ let autoProducts = [swiftPMProduct, swiftPMDataModelProduct]
let package = Package(
name: "SwiftPM",
platforms: [
.macOS(.v12),
.macOS(.v13),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also update iOS version to v16 for alignment?

Kyle-Ye
Kyle-Ye reviewed Jul 12, 2023
View reviewed changes
Copy link
Contributor

@Kyle-Ye Kyle-Ye left a comment

Choose a reason for hiding this comment

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

LTGM. But I was wondering whether it is the appropriate time to bump the minimal version.

See #6598 (comment)

@Kyle-Ye
Copy link
Contributor

Kyle-Ye commented Jul 12, 2023

agreed - I think that can be achieved by changing the package min deployment platform back to 12 and adding inline availability checks until we can raise the min deployment target

-1 for this. It seems just increase the code complexity. I'd suggest we merge this after we can safely bump the minimal corresponding OS version.

@tomerd
Copy link
Contributor

tomerd commented Jul 12, 2023

that may take a while which means this will bit rot. the increased complexity is a good argument, but its a tradeoff

@neonichu neonichu added the next waiting for next merge window label Sep 5, 2023
@Kyle-Ye
Copy link
Contributor

Kyle-Ye commented Feb 25, 2025

With #7313 is merged now, I think there is no blocking items for this PR. Would you mind updating the PR and fix the conflicts so that we can land this? @compnerd

@compnerd
Copy link
Member Author

@swift-ci please test

@compnerd
Remove use of NSString.replacingOccurrences(of:with:)
ca536f8 
Prefer to use `String.replacing(_:with:)` instead of the `NSString`
operations for the strings.  This should avoid an unnecessary use of
Foundation API and a dispatch to the `NSString` API.

Bump the minimum platform version to 10.13 to deal with the API
availability in the standard library.
@compnerd
Copy link
Member Author

compnerd commented Mar 3, 2025

@swift-ci please test

@plemarquand
Copy link
Contributor

@swift-ci please test windows

@tomerd tomerd merged commit 124127e into swiftlang:main Mar 4, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kyle-Ye Kyle-Ye Kyle-Ye left review comments

@MaxDesiatov MaxDesiatov MaxDesiatov approved these changes

@neonichu neonichu neonichu approved these changes

@abertelrud abertelrud Awaiting requested review from abertelrud

@tomerd tomerd Awaiting requested review from tomerd

@bnbarham bnbarham Awaiting requested review from bnbarham

@jakepetroules jakepetroules Awaiting requested review from jakepetroules jakepetroules is a code owner

@xedin xedin Awaiting requested review from xedin

@dschaefer2 dschaefer2 Awaiting requested review from dschaefer2 dschaefer2 is a code owner

@shawnhyam shawnhyam Awaiting requested review from shawnhyam shawnhyam is a code owner

@bripeticca bripeticca Awaiting requested review from bripeticca bripeticca is a code owner

@plemarquand plemarquand Awaiting requested review from plemarquand plemarquand is a code owner

@owenv owenv Awaiting requested review from owenv owenv is a code owner

@bkhouri bkhouri Awaiting requested review from bkhouri bkhouri is a code owner

Assignees
No one assigned
Labels
next waiting for next merge window
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@compnerd @MaxDesiatov @tomerd @neonichu @Kyle-Ye @plemarquand