-
Notifications
You must be signed in to change notification settings - Fork 30
[RFC 9651] Add support for Display String type to RawStructuredFieldValues #43
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
[RFC 9651] Add support for Display String type to RawStructuredFieldValues #43
Conversation
…alues (apple#41) Motivation: [RFC 9651](https://www.ietf.org/rfc/rfc9651.html) added the Display String Structured Type. Modifications: - Implement the parser and serializer for Display String in the RawStructuredFieldValues module. Result: The RawStructuredFieldValues module will support the Display String type.
|
||
let startIndex = self.underlyingData.startIndex | ||
let secondIndex = self.underlyingData.index(after: startIndex) | ||
let octetHex = self.underlyingData[...secondIndex] |
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.
We can skip the index math and do let octetHex = self.underlyingData.prefix(2)
self.underlyingData = self.underlyingData.dropFirst(2) | ||
|
||
guard | ||
octetHex.allSatisfy({ asciiDigits.contains($0) || asciiLowercases.contains($0) }), |
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.
This would be somewhere I'd write a little micro-extension method for this purpose, as we know that there are exactly two values and so we don't strictly need a loop.
|
||
extension UInt8 { | ||
/// Converts a hex value given in UTF8 to base 10. | ||
fileprivate static func decodeHex<Bytes: RandomAccessCollection>(_ bytes: Bytes) -> Self? |
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.
This is also excessively generic I think. We only ever accept a single data type, and it's always two bytes long. It might actually be useful to express that as its own type, e.g:
struct EncodedHex {
var first: UInt8
var second: UInt8
init(_ slice: ArraySlice<UInt8>) {
precondition(slice.count == 2)
self.first = slice[slice.startIndex]
self.second = slice[slice.index(after: slice.startIndex)]
}
}
That lets us express the rest of this math in a much less branchy way, and with an avoidance of preconditions elsewhere.
/// Converts a hex character given in UTF8 to its integer value. | ||
private static func htoi(_ value: Self) -> Self? { | ||
let charA = Self(UnicodeScalar("a").value) | ||
let char0 = Self(UnicodeScalar("0").value) |
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.
Prefer the UInt8(ascii:)
initializer.
let char0 = Self(UnicodeScalar("0").value) | ||
|
||
switch value { | ||
case char0...char0 + 9: |
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.
These are a touch nicer using named values at both ends of the range.
|| (0x7F...).contains(byte) | ||
{ | ||
self.data.append(asciiPercent) | ||
self.data.append(contentsOf: String(byte, radix: 16, uppercase: false).utf8) |
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 think we can avoid constructing a String here, if we write the code out.
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.
Great, just a few quick notes and then we're good. Thanks for this!
throw StructuredHeaderError.invalidDisplayString | ||
} | ||
|
||
let octetHex = EncodedHex(ArraySlice(self.underlyingData.prefix(2))) |
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 the ArraySlice
constructor should be necessary.
let octetHex = EncodedHex(ArraySlice(self.underlyingData.prefix(2))) | |
let octetHex = EncodedHex(self.underlyingData.prefix(2)) |
try $0.withMemoryRebound(to: CChar.self) { | ||
guard let baseAddress = $0.baseAddress else { | ||
throw StructuredHeaderError.invalidDisplayString | ||
} |
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.
You don't need this check: the base address will never be nil
here.
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.
Cool, this LGTM. Nice one @clintonpi!
API breakage is expected, merging over it. |
Motivation:
RFC 9651 added the Display String Structured Type.
Modifications:
Result:
The RawStructuredFieldValues module will support the Display String type.