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

Jump to bottom

[SR-10094] withContiguousStorageIfAvailable(_:) needs documentation #52496

Open
lilyball mannequin opened this issue Mar 12, 2019 · 6 comments
Open

[SR-10094] withContiguousStorageIfAvailable(_:) needs documentation #52496

lilyball mannequin opened this issue Mar 12, 2019 · 6 comments
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. documentation standard library Area: Standard library umbrella

Comments

@lilyball
Copy link
Mannequin

lilyball mannequin commented Mar 12, 2019

Previous ID SR-10094
Radar None
Original Reporter @lilyball
Type Bug
Additional Detail from JIRA
Votes 0
Component/s Standard Library
Labels Bug, Documentation
Assignee fassko (JIRA)
Priority Medium

md5: e077a76eeecc3dc024d0dd80f9cd4f60

Issue Description:

SE-0237 introduces Sequence.withContiguousStorageIfAvailable(_:) and Collection.withContiguousMutableStorageIfAvailable(_:). Neither the protocol methods nor any of the concrete implementations have documentation, and they need it.

Of special note is String.UTF8View.withContiguousStorageIfAvailable(_:). SE-0237 documents that withContiguousStorageIfAvailable(_:) may allocate contiguous storage if it doesn't yet exist. The current implementation of String.UTF8View.withContiguousStorageIfAvailable(_:) checks if the String is already backed by UTF-8 and returns nil if not. Based on the proposal description, this implementation could instead allocate a temporary buffer that copies the bridged string to UTF-8. The current behavior is preferable, but as an external developer I feel like I can't rely on it until it's documented, so this particular implementation should explicitly document that it returns nil if the string isn't already backed by UTF-8.
@belkadan
Copy link
Contributor

cc @natecook1000, @lorentey

@swift-ci
Copy link
Collaborator

swift-ci commented Jun 7, 2019

Comment by Kristaps Grinbergs (JIRA)

I submitted this in PR #25301

@swift-ci
Copy link
Collaborator

swift-ci commented Jul 5, 2019

Comment by Kristaps Grinbergs (JIRA)

What would be the right approach with this issue as my PR is merged? Close or resolve the issue?

@belkadan
Copy link
Contributor

belkadan commented Jul 5, 2019

The person who fixes it marks it Resolved; the person who reported it gets to Close it if they're satisfied.

@lilyball
Copy link
Mannequin Author

lilyball mannequin commented Jul 5, 2019

Speaking as the person who reported this, after looking at the PR, that's good progress but I'm not satisfied. In particular, the second paragraph of this bug is talking about String.UTF8View.withContiguousStorageIfAvailable(_:) and the linked PR adds no documentation to that concrete implementation, so the concerns I raise are still valid (in particular, I want assurances that calling this on String.UTF8View won't convert the string to UTF-8 if it's not already backed by UTF-8 storage).

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
glessard added a commit to glessard/swift that referenced this issue Jul 12, 2023
@glessard
[doc] update documentation for UTF8View.wCSIA()
e7552d6 
- We can safely state that when contiguous storage does not exist, none will get allocated.

Addresses apple#52496
@glessard
Copy link
Contributor

glessard commented Jul 13, 2023 •
edited

In practice, no type in the standard library makes an allocation in order to provide a contiguous buffer through withContiguousStorageIfAvailable (wCSIA). The standard library also relies on wCSIA to provide fast paths for many algorithms, and to temporarily manifest storage would defeat this.

Should we improve the documentation to clearly state a performance expectation for wCSIA, and to remove the suggestion that temporary memory allocations may occur? We earlier altered the documentation to make it clear that wCSIA should provide access to all of a Collection's elements, in order to clarify the performance expectations. Disallowing (or at least discouraging) temporary memory allocations would be along the same lines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. documentation standard library Area: Standard library umbrella
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@glessard @belkadan @swift-ci