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

Fix memory leak in CFURL.c #2899

Merged
merged 2 commits into from
Jun 9, 2021
Merged

Fix memory leak in CFURL.c #2899

merged 2 commits into from
Jun 9, 2021

Conversation

valeriyvan
Copy link
Contributor

No description provided.

@MaxDesiatov
Copy link
Contributor

@swift-ci please test

compnerd
compnerd requested changes Oct 10, 2020
View reviewed changes
Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

This needs a better commit message. Additionally, it is unclear from the code that the POSIXPathToURLPath returns a +1'ed object as it does not conform to the CF naming scheme. I think that it should at least be commented to explain why this needs changing.

@valeriyvan
Copy link
Contributor Author

valeriyvan commented Oct 10, 2020 •
edited
Loading

Additionally, it is unclear from the code that the POSIXPathToURLPath returns a +1'ed object as it does not conform to the CF naming scheme.

POSIXPathToURLPath is annotated as CF_RETURNS_RETAINED

@valeriyvan
Copy link
Contributor Author

This needs a better commit message.

Updated commit message

@valeriyvan valeriyvan requested a review from compnerd October 10, 2020 21:57
compnerd
compnerd reviewed Nov 1, 2020
View reviewed changes
CoreFoundation/URL.subproj/CFURL.c
result = _CFURLCreateWithArbitraryString(allocator, newString, url->_base);
}
if (newComp) CFRelease(newComp);
Copy link
Member

Choose a reason for hiding this comment

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

I suppose that all paths are guaranteed to initialize newComp, but I think it would be easier to follow with an explicit nullptr initialization at L4789.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@MaxDesiatov
Copy link
Contributor

@swift-ci please test

@MaxDesiatov MaxDesiatov requested a review from compnerd November 2, 2020 16:36
@valeriyvan
Copy link
Contributor Author

ping

@spevans
Copy link
Contributor

spevans commented Jun 8, 2021

@swift-ci test and merge

1 similar comment
@spevans
Copy link
Contributor

spevans commented Jun 9, 2021

@swift-ci test and merge

@swift-ci swift-ci merged commit 3cf6ce3 into swiftlang:main Jun 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@parkera parkera parkera approved these changes

@millenomi millenomi Awaiting requested review from millenomi

@compnerd compnerd Awaiting requested review from compnerd

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

Successfully merging this pull request may close these issues.
6 participants
@valeriyvan @MaxDesiatov @spevans @compnerd @parkera @swift-ci