Skip to content

[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

Merged
merged 7 commits into from
Nov 22, 2024

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