Skip to content

Conversation

@MaxDesiatov
Copy link
Contributor

@MaxDesiatov MaxDesiatov commented Feb 18, 2022

This implementation excludes a few properties that rely on precondition, which will be submitted as a separate PR.

}

@_transparent
public static func &= (lhs: inout UInt8, rhs: UInt8) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These operators are required for StaticString flag checks. I'll move them to a corresponding numeric protocol conformance in a future PR.

@MaxDesiatov
Copy link
Contributor Author

MaxDesiatov commented Feb 18, 2022

Build failure due to

StaticString.swift.obj : error LNK2019: 
unresolved external symbol swift_getTypeByMangledNameInContext referenced in function __swift_instantiateConcreteTypeFromMangledName

StaticString.swift.obj : error LNK2019: 
unresolved external symbol swift_getWitnessTable referenced in function $sSPys5UInt8VGSPyxGs8_PointersWl

StaticString.swift.obj : error LNK2019: 
unresolved external symbol swift_getTypeByMangledNameInContextInMetadataState referenced in function __swift_instantiateConcreteTypeFromMangledNameAbstract

$sSPys5UInt8VGSPyxGs8_PointersWl demangles to lazy protocol witness table accessor for type Swift.UnsafePointer<Swift.UInt8> and conformance Swift.UnsafePointer<A> : Swift._Pointer in Swift

I wonder if there's some way to specialize UnsafePointer<UInt8> to work around this?

@MaxDesiatov
Copy link
Contributor Author

MaxDesiatov commented Feb 18, 2022

Tried to monomorphize that call to UnsafePointer(bitPattern:) by adding @inline(__always), but that didn't help, and @_specialize(exported: true, kind: full, where Pointee == UInt8) can apparently only be applied to generic functions?

@MaxDesiatov
Copy link
Contributor Author

Ok, I removed utf8Start for now, but looks like code in OpaquePointer is still too complex for the release versions of the compiler 🤷‍♂️

@compnerd
Copy link
Owner

compnerd commented Feb 18, 2022

I tend to prefer that the changes are focused rather than a grab bag of changes. That makes it too difficult to focus on one thing. Please split this up accordingly.

Additionally, I'm not sure I want to add CMake toolchain configurations, especially the way that you have setup the cache file. It encodes much about the build environment. For example, I wish to build on Windows with MSVC 2022 to target ARM but that cache file doesn't work :) (or perhaps I want to build on exherbo Linux).

@MaxDesiatov
Copy link
Contributor Author

MaxDesiatov commented Feb 18, 2022

I tend to prefer that the changes are focused rather than a grab bag of changes. That makes it too difficult to focus on one thing. Please split this up accordingly.

Sure, I can move out the .gitignore commit to a separate PR, but StaticString and pointer additions would have to be bundled, is that ok?

What about new metadata requirements, would you like utf8Start implementation to be postponed? This should not imply adding any Unicode/ICU dependencies, but apparently utf8Start is required to iterate over characters in StaticString for printing and such. And this raises a more general point of generics and metadata. Should we avoid generics altogether for now, and hide APIs requiring generics behind a flag? Is there any other way to prevent these dependencies on metadata symbols from being emitted?

@compnerd
Copy link
Owner

I think that we can split out the pointer changes, assertion changes, and static string changes. StaticString is definitely a type that I would like to see sooner rather than later, but splitting up the changes makes it easier to test.

As to the build failures, I don't think its the end of the world if we cannot support 5.4, but it would be nice to retain support for 5.5 - stable, development would be nice. If we cannot manage to do that, I'd be willing to settle for 5.6 + main.

@MaxDesiatov MaxDesiatov marked this pull request as draft February 19, 2022 11:26
@MaxDesiatov MaxDesiatov marked this pull request as ready for review February 19, 2022 21:55
}

public protocol _ExpressibleByBuiltinStringLiteral {
init(
Copy link
Owner

Choose a reason for hiding this comment

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

Please don't break immediately after the (, prefer to bin-pack the arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What column limit would you like to set? Would something like 100 or 120 work?

) {
_startPtrOrData = Builtin.ptrtoint_Word(_start)
_utf8CodeUnitCount = utf8CodeUnitCount
_flags = Bool(isASCII)
Copy link
Owner

Choose a reason for hiding this comment

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

This took me a second to understand. Can you add a comment to the _flags ivar to indicate what the bit flags are and what is reserved?


@_transparent
public var isASCII: Bool {
(UInt8(_flags) & 0x2) != 0
Copy link
Owner

Choose a reason for hiding this comment

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

Please use explicit returns.

@MaxDesiatov MaxDesiatov requested a review from compnerd February 20, 2022 14:57
@MaxDesiatov MaxDesiatov changed the title Add StaticString and required pointer types Add StaticString implementation Feb 20, 2022
@usableFromInline
internal var _utf8CodeUnitCount: Builtin.Word

/// Flags indicating how `StaticString` storage is used.
Copy link
Owner

Choose a reason for hiding this comment

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

Having additional documentation of reserved and in use fields would be desirable.

extension StaticString: _ExpressibleByBuiltinStringLiteral {
@_effects(readonly)
@_transparent
public init(
Copy link
Owner

Choose a reason for hiding this comment

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

please dont break after (

Copy link
Owner

@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.

Please stick to 80-column and bin-packing arguments (and not breaking after ().

@compnerd compnerd merged commit 60aa4b3 into compnerd:main Feb 20, 2022
@MaxDesiatov MaxDesiatov deleted the maxd/static-string branch February 20, 2022 19:33
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.

2 participants