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

Jump to bottom

Replace calls to CFStringCreateWithCString with constant CFStrings #4694

Merged
merged 1 commit into from
Mar 16, 2023
Merged

Replace calls to CFStringCreateWithCString with constant CFStrings #4694

merged 1 commit into from
Mar 16, 2023

Conversation

AZero13
Copy link
Contributor

@AZero13 AZero13 commented Jan 20, 2023

This would save performance by removing unnecessary calls to CFStringCreateWithCString

@AZero13
Copy link
Contributor Author

AZero13 commented Jan 23, 2023

@compnerd Is this good?

@compnerd
Copy link
Member

I'm conflicted on this. This is diverging CoreFoundation, but not for fixing bugs. But on the other hand, we have not updated the CoreFoundation in use here for a while. Plus, with What's next for Foundation, I expect that this code will not live too far into the future. So is this really worth it?

@compnerd
Copy link
Member

CC: @parkera

@AZero13 AZero13 force-pushed the CFStringCreateWithCString branch from a2a752e to 43c1c59 Compare January 24, 2023 15:33
parkera
parkera requested changes Jan 24, 2023
View reviewed changes
Copy link
Contributor

@parkera parkera left a comment

Choose a reason for hiding this comment

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

I think using constant strings here is fine. One code style fix that needs to be made first before merging.

@AZero13
Copy link
Contributor Author

AZero13 commented Jan 24, 2023

@parkera Done!

@AZero13 AZero13 force-pushed the CFStringCreateWithCString branch from 43c1c59 to fe1098d Compare January 24, 2023 18:00
@AZero13 AZero13 requested a review from parkera January 24, 2023 18:01
@parkera
Copy link
Contributor

parkera commented Jan 24, 2023

@swift-ci test

@AZero13 AZero13 force-pushed the CFStringCreateWithCString branch 2 times, most recently from 313e5a4 to ee7a570 Compare January 25, 2023 16:38
@AZero13
Copy link
Contributor Author

AZero13 commented Jan 25, 2023 •
edited
Loading

@compnerd Could you please run the tests again and merge?

@compnerd
Copy link
Member

@swift-ci please test

@AZero13
Copy link
Contributor Author

AZero13 commented Jan 25, 2023

@compnerd Seems like the macOS Jenkins instance is broken; it will not start the pipeline

@AZero13 AZero13 force-pushed the CFStringCreateWithCString branch 4 times, most recently from 3a0d9e1 to 59c71f7 Compare January 29, 2023 01:34
@AZero13 AZero13 force-pushed the CFStringCreateWithCString branch from 59c71f7 to 428cb5f Compare February 2, 2023 21:58
@parkera
Copy link
Contributor

parkera commented Feb 27, 2023

@swift-ci test

@AZero13 AZero13 force-pushed the CFStringCreateWithCString branch from 428cb5f to 84dd1ab Compare March 1, 2023 15:24
@AZero13
Copy link
Contributor Author

AZero13 commented Mar 1, 2023

@parkera Everything is fixed. Can we do this one more time?

@AZero13 AZero13 force-pushed the CFStringCreateWithCString branch from 84dd1ab to 8faff37 Compare March 1, 2023 16:34
@AZero13
Copy link
Contributor Author

AZero13 commented Mar 1, 2023

@compnerd Could you please run the tests again and merge?

@AZero13
Replace calls to CFStringCreateWithCString with constant CFStrings
10b2bd4 
This would save performance by removing unnecessary calls to CFStringCreateWithCString.
@AZero13 AZero13 force-pushed the CFStringCreateWithCString branch from 8faff37 to 10b2bd4 Compare March 5, 2023 17:44
@AZero13
Copy link
Contributor Author

AZero13 commented Mar 14, 2023

Can someone please do the please test and merge thing? @compnerd @parkera

@parkera
Copy link
Contributor

parkera commented Mar 14, 2023

@swift-ci test

@AZero13
Copy link
Contributor Author

AZero13 commented Mar 15, 2023

@swift-ci test

It passed! Can we merge please? @parkera

@parkera parkera merged commit bc34587 into swiftlang:main Mar 16, 2023
@parkera
Copy link
Contributor

parkera commented Mar 16, 2023

Thank you!

@AZero13 AZero13 deleted the CFStringCreateWithCString branch March 16, 2023 19:45
@compnerd
Copy link
Member

This might be regressing the Windows build:
https://ci-external.swift.org/job/oss-swift-windows-toolchain-x86_64-vs2019/2845/console

compnerd added a commit that referenced this pull request Mar 16, 2023
@compnerd
Revert "Replace calls to CFStringCreateWithCString with constant CFSt…
22e9ff2 
…rings (#4694)"

This reverts commit bc34587.
@compnerd compnerd mentioned this pull request Mar 16, 2023
Merged
compnerd added a commit that referenced this pull request Mar 16, 2023
@compnerd
Revert "Replace calls to CFStringCreateWithCString with constant CFSt…
74226d1 
…rings (#4694)"

This reverts commit bc34587.
@AZero13 AZero13 restored the CFStringCreateWithCString branch March 17, 2023 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@parkera parkera

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AZero13 @compnerd @parkera