-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
[embedded] Add a mechanism to opt-in for using String, and for using Unicode data tables #73682
Conversation
…Unicode data tables
@@ -894,6 +894,13 @@ def no_allocations : Flag<["-"], "no-allocations">, | |||
Flags<[FrontendOption, HelpHidden]>, | |||
HelpText<"Diagnose any code that needs to heap allocate (classes, closures, etc.)">; | |||
|
|||
def enable_strings : Flag<["-"], "enable-strings">, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would -enable-embedded-strings
work? It's not a big deal, but -enable-strings
seems a bit overly generic to me at first glance, unless we can think of other scenarios where String
would be restricted outside of -embedded
mode (and we could always introduce a more generic option later).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want flags unique to embedded if we can avoid it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strings are implicitly available by default, so unless you are in embedded mode this flag doesn't make sense. In this phrasing, how can it not be specific to embedded? In order to make it generic, I think it would have to be -disable-strings
, which could theoretically work in any configuration (but it's unclear why you'd use it outside of embedded).
extension Character: Equatable { | ||
@inlinable @inline(__always) | ||
@_effects(readonly) | ||
@_needsUnicodeDataTablesInEmbedded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order for this to be intuitive for library authors, I think we should probably aim to support lexical scoping for this attribute so that you don't have to repeat it like this
I have some serious concerns about the split here; having four levels of configuration is going to have some severe impact to libraries/shared-code above this split. It feels like we are kinda punting a design problem out to our users. Perhaps we should take some time and review this a bit more one-on-one to figure out how we can tackle it a bit more ergonomically and perhaps isolate some areas that need improvement by other teams. |
👍 |
I'm going to shelve/abandon this approach, and avoid introducing any splits or levels, at least for now. But I'm going to proceed with porting Swift.String to Embedded Swift (#70446) as that's currently the largest missing feature compared to full Swift. I'll also start some more discussions about strings and Swift.String, and also about "opt-in" and "opt-out" or other possibilities to enforce usage/non-usage of strings in embedded contexts. |
Building on top the previous parts of porting String into Embedded Swift (#70446, #73585), this PR adds a mechanism to select whether an Embedded Swift program wants to use (dynamic) Strings or not, and whether it wants to use Unicode data tables that are needed for operations like comparing strings for equality, or using strings as keys in a dictionary.
Background + motivation:
Approach in this PR
-enable-strings
:-enable-string=full-unicode-data-tables
gets us into level 2.