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

[SR-10244] Compression overlay #52644

Closed
benrimmington opened this issue Mar 30, 2019 · 6 comments
Closed

[SR-10244] Compression overlay #52644

benrimmington opened this issue Mar 30, 2019 · 6 comments
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior.

Comments

@benrimmington
Copy link
Collaborator

Previous ID SR-10244
Radar rdar://problem/48326535
Original Reporter @benrimmington
Type Bug
Status Resolved
Resolution Done
Additional Detail from JIRA
Votes 0
Component/s
Labels Bug
Assignee None
Priority Medium

md5: 92f37e58a29d0ef9552d7c726fa159a9

Issue Description:

An overlay for the Compression framework was added in apple/swift#21939 (for Swift 5.1).

The internal compression_stream initializer is allocating empty buffers using UnsafeMutablePointer<UInt8>.allocate(capacity:0) but these are not deallocated later. The compression_stream_init function will reset dst_ptr and src_ptr to nil. I suspect that the overlay will leak minimum-sized allocations.

  • Can the dst_ptr and src_ptr be changed to __nullable in the <compression.h> file?

  • Or can the internal initializer use UnsafeMutablePointer<UInt8>(bitPattern: -1)! instead?

The framework uses typed uint8_t * buffer pointers, but Foundation.Data prefers raw void * buffer pointers. The overlay appears to be using deprecated withUnsafeBytes and withUnsafeMutableBytes methods.

  • Can the dst_ptr and src_ptr be changed to void * and const void * in the <compression.h> file?

  • Or can the overlay use non-deprecated withUnsafeBytes and withUnsafeMutableBytes methods, followed by some kind of rebinding to typed pointers?

The overlay also adds a FilterError enum. Some of its case names have redundant parts (i.e. the filter prefixes and Error suffixes). Some of the documentation comments in the overlay are referring to older case names.

  • /// - Throws: `FilterError.StreamInitError` if stream initialization failed

  • /// - Throws: `FilterError.StreamProcessError` if an error occurs during processing

The availability of C and Swift APIs is different:

  • __API_AVAILABLE(macos(10.11), ios(9.0)) in the <compression.h> file.

  • @available(macOS 10.12, iOS 10.0, watchOS 3.0, tvOS 10.0, *) in the overlay.

  • Should the <compression.h> file also have tvOS and watchOS version numbers?

  • Should the overlay use version 9999 placeholders?

Could the <compression.h> file use attributes for ClangImporter, rather than adding wrappers to the overlay?

  • attribute((swift_name("..."))) for the enums and their cases.

  • attribute((enum_extensibility(closed))) for the compression_stream_operation and compression_status enums.

  • Deprecated typealiases, etc. in the overlay for source compatibility.

If the overlay imports Foundation, does that mean that Foundation can't import Compression later, due to a circular dependency? For example, SE-0088 was unable to use Foundation.Date due to "layering restrictions".

  • Should the InputFilter and OutputFilter classes be moved to Foundation, possibly as subclasses of the abstract Stream class?

  • Or could the overlay use a generic requirement similar to MutableDataProtocol?

@benrimmington
Copy link
Collaborator Author

Cc: @@moiseev

@moiseev
Copy link
Mannequin

moiseev mannequin commented Apr 2, 2019

Thanks for reporting this, @benrimmington!

I am not entirely sure bugs like this belong here, though. In general problems with the Apple APIs should be reported via https://bugreport.apple.com/

For a better discoverability it might make sense to create a bug here and link to a radar filed at the URL above.

@moiseev
Copy link
Mannequin

moiseev mannequin commented Apr 2, 2019

Oh.. Just to make sure, I contacted the Compression team and sent them your suggestions, so you don't need to file radars for this particular issue.

@benrimmington
Copy link
Collaborator Author

@@moiseev, thanks for contacting the team. I've updated the description of this issue, but I can also file radars if needed. The overlay is already in the Swift 5.1 branch, so it might be too late to fix anything.

@benrimmington
Copy link
Collaborator Author

apple/swift#23856

@benrimmington
Copy link
Collaborator Author

Fixed by Eric Bainville in:

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior.
Projects
None yet
Development

No branches or pull requests

1 participant