Skip to content

Conversation

clintonpi
Copy link
Contributor

Motivation:

RFC 9651 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.

…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.
@Lukasa Lukasa added the 🆕 semver/minor Adds new public API. label Nov 15, 2024

let startIndex = self.underlyingData.startIndex
let secondIndex = self.underlyingData.index(after: startIndex)
let octetHex = self.underlyingData[...secondIndex]
Copy link
Contributor

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) }),
Copy link
Contributor

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?
Copy link
Contributor

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)
Copy link
Contributor

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:
Copy link
Contributor

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)
Copy link
Contributor

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.

@clintonpi clintonpi requested a review from Lukasa November 20, 2024 10:25
Copy link
Contributor

@Lukasa Lukasa left a 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)))
Copy link
Contributor

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.

Suggested change
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
}
Copy link
Contributor

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.

@clintonpi clintonpi requested a review from Lukasa November 22, 2024 10:22
Copy link
Contributor

@Lukasa Lukasa left a 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!

@Lukasa
Copy link
Contributor

Lukasa commented Nov 22, 2024

API breakage is expected, merging over it.

@Lukasa Lukasa merged commit c2e79ce into apple:main Nov 22, 2024
25 of 26 checks passed
@clintonpi clintonpi deleted the rfc-9651/raw-support-for-display-string branch November 25, 2024 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🆕 semver/minor Adds new public API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants