Skip to content

[stdlib] Re-add withContiguousStorageIfAvailable to SubString.UTF8View #29146

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

Merged
merged 1 commit into from
Jan 22, 2020

Conversation

Lukasa
Copy link
Contributor

@Lukasa Lukasa commented Jan 11, 2020

This is a second pass at the original patch, which broke an OS test. Originally added in #29094 and reverted in #29128. /cc @milseman

Due to an oversight it seems that we never added a
withContigousStorageIfAvailable implementation to SubString.UTF8View,
which meant that if you sliced a String you lost the ability to get fast
access to the backing storage. There's no good reason for this
functionality to be missing, so this patch adds it in by delegating to
the Slice implementation.

Resolves SR-11999. rdar://problem/58663804

@Lukasa
Copy link
Contributor Author

Lukasa commented Jan 11, 2020

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - c69cadae61d44191a087136d758dcff6b4667009

@Lukasa
Copy link
Contributor Author

Lukasa commented Jan 11, 2020

Ok, I definitely need a Swift expert to help me here, because I can't decipher that failure at all. This failure seems to be pretty far away from the change I made, though I admit that that doesn't mean all that much.

@theblixguy
Copy link
Collaborator

I think the DebugInfo test failure is fixed by #29149, but I’m not sure about the other one.

@airspeedswift
Copy link
Member

@swift-ci please test macOS platform

@Lukasa
Copy link
Contributor Author

Lukasa commented Jan 13, 2020

Thanks @airspeedswift. Ok, so is our conclusion that the full test suite does not catch the failure that caused us to back this out the first time @milseman?

@milseman
Copy link
Member

The failure was here, which should build the test with the new compiler+stdlib, and run with the old stdlib. Your change should work if alwaysEmitIntoClient is working, IIUC.

@milseman
Copy link
Member

@slavapestov would this be a correct use of alwaysEmitIntoClient?

@milseman
Copy link
Member

milseman commented Jan 13, 2020

Doh, I missed the fact that the test is generic (thanks @aschwaighofer). @Lukasa, can you have a concrete test and a generic one? The concrete one should always pass, but since witness tables are constructed at compile time, the generic one should only pass on bleeding-edge.

@lorentey, what would be the preferred way for Cory to differentiate between master and a shipped OS? Would that be something like:

  let hasSubstringWCSIA: Bool
  if #available(macOS 9999, iOS 9999, watchOS 9999, tvOS 9999, *) {
    hasSubstringWCSIA = true 
  } else {
    hasSubstringWCSIA = false
  }
  ... 

@Lukasa Lukasa force-pushed the cb-substring-fast-access-2 branch from c69cada to e458538 Compare January 14, 2020 12:51
@Lukasa
Copy link
Contributor Author

Lukasa commented Jan 14, 2020

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - c69cadae61d44191a087136d758dcff6b4667009

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - c69cadae61d44191a087136d758dcff6b4667009

This is a second pass at the original patch, which broke an OS test.

Due to an oversight it seems that we never added a
withContigousStorageIfAvailable implementation to SubString.UTF8View,
which meant that if you sliced a String you lost the ability to get fast
access to the backing storage. There's no good reason for this
functionality to be missing, so this patch adds it in by delegating to
the Slice implementation.

Resolves SR-11999.
@Lukasa Lukasa force-pushed the cb-substring-fast-access-2 branch from e458538 to c6dfea6 Compare January 14, 2020 14:01
@Lukasa
Copy link
Contributor Author

Lukasa commented Jan 14, 2020

@swift-ci please smoke test

@Lukasa
Copy link
Contributor Author

Lukasa commented Jan 14, 2020

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - e458538212e89a520c039ec6048ee087c57102eb

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - e458538212e89a520c039ec6048ee087c57102eb

@varungandhi-apple
Copy link
Contributor

That test failure is non-deterministic and has been disabled for now.

@swift-ci please test

@lorentey
Copy link
Member

@lorentey, what would be the preferred way for Cory to differentiate between master and a shipped OS? Would that be something like:

  let hasSubstringWCSIA: Bool
  if #available(macOS 9999, iOS 9999, watchOS 9999, tvOS 9999, *) {
    hasSubstringWCSIA = true 
  } else {
    hasSubstringWCSIA = false
  }
  ... 

Yeah, unfortunately the best we can do at the moment is to tie it to 9999-availability. The 9999s will get (manually) updated into actual version numbers when this reaches production compilers, which isn't exactly what we want, but we don't have a good way to test for custom-stdlib-or-recent-enough-OS yet. :-(

@lorentey
Copy link
Member

Come to think of it -- we should just a boolean flag to the stdlib that indicates if it's a production build, and then we can implement the correct condition. (Annoyingly, #available doesn't support other clauses in the same if statement, so we'd have to do a nested check somehow.)

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - c6dfea6

@Lukasa
Copy link
Contributor Author

Lukasa commented Jan 15, 2020

Hmm, the OSX failure here seems unrelated.

@Lukasa
Copy link
Contributor Author

Lukasa commented Jan 15, 2020

@swift-ci please test

@Lukasa
Copy link
Contributor Author

Lukasa commented Jan 16, 2020

Ok, where do we stand here now @milseman? Are we happy to try merging again?

@milseman
Copy link
Member

Sorry I missed the notification. LGTM

@milseman milseman merged commit 38de918 into swiftlang:master Jan 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants