-
Notifications
You must be signed in to change notification settings - Fork 52
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
Add conveniences for modern HTTP parsers #54
Conversation
@swift-ci test |
} | ||
guard token.utf8.allSatisfy({ | ||
switch $0 { | ||
case 0x21, 0x23, 0x24, 0x25, 0x26, 0x27, 0x2A, 0x2B, 0x2D, 0x2E, 0x5E, 0x5F, 0x60, 0x7C, 0x7E: |
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.
Should we define these somewhere so that it can match against human-readable names to reduce risk of missing something?
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 HTTPField.isValidToken
minus capital letters. I can't think of an easy way to share without incurring additional performance overhead. My original change was name == name.lowercased()
but that could require allocation.
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 can use UInt8(ascii: "")
for each one. It'll get a bit more verbose, mind.
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 can make a separate change to switch everything to that after landing this.
} | ||
|
||
var request: HTTPRequest { | ||
mutating get throws { |
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.
Why is this and the response mutating?
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.
Oh that was leftover from the internal code where we actually reset the struct into the initial state so it can be reused to accumulate more fields. I removed them.
} | ||
} | ||
|
||
var request: HTTPRequest { |
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.
NIT: We often write these as init
s on the types and do the logic in the init instead of making them "conversion" vars. This follows how Swift defines type conversions
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.
The ultimate APIs are inits, but I thought they are better as computed properties here so we can keep all the state private.
} | ||
guard token.utf8.allSatisfy({ | ||
switch $0 { | ||
case 0x21, 0x23, 0x24, 0x25, 0x26, 0x27, 0x2A, 0x2B, 0x2D, 0x2E, 0x5E, 0x5F, 0x60, 0x7C, 0x7E: |
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 can use UInt8(ascii: "")
for each one. It'll get a bit more verbose, mind.
@swift-ci test |
The following methods are added for adopting HTTPTypes within modern HTTP parsers (HPACK / QPACK).
Resolves rdar://129983286