httpwg / http-extensions

HTTP Extensions in progress
https://httpwg.org/http-extensions/
447 stars 147 forks source link

SH nits #961

Closed mnot closed 5 years ago

mnot commented 5 years ago
mnot commented 5 years ago

@phluid61 was it you that pointed out the first bullet above? I remember a conversation about it, but I don't remember where it was. Simple fix is to change to 1*SP, I think...

phluid61 commented 5 years ago

Yep it was me, in a comment in PR 954. The algorithm currently accepts SP OWS.

mnot commented 5 years ago

SP OWS sounds most consistent with what's already there. Thanks.

mnot commented 5 years ago

Since token is required to start with ALPHA, is there any reason we can't just allow all RFC7230 token characters after that?

phluid61 commented 5 years ago

tchar doesn't include slash / or colon :

mnot commented 5 years ago

Good catch.

phluid61 commented 5 years ago

Having to include separators kinda highlights the fact that we're using sh-token for things that are a bit more structured.

clelland commented 5 years ago

Re: Point 1: it looks like the ABNF allows OWS before the closing ')' on an inner list, but the parsing algorithm only accepts a single SP (Parsing an inner list, step 3.v)

phluid61 commented 5 years ago

You want to add this?

inner-list = "(" OWS [ sh-item *( SP OWS sh-item ) [ SP OWS ] ] ")" *parameter
                                                   ^^^^     ^
martinthomson commented 5 years ago

Why at the end and not also at the beginning?

phluid61 commented 5 years ago

It's after an sh-item. The algorithm is: OWS, item, space-or-paren, goto 1.

At this point it's probably easier and more sensible to just add HTAB to the allowed characters in 3.v, and change the ABNF to "(" OWS [ sh-item *( RWS sh-item ) OWS ] ")"

clelland commented 5 years ago

@phluid61, yes, the issue was that the ABNF looks like inner-list = "(" OWS [ sh-item *( SP OWS sh-item ) OWS ] ")" *parameter

but what is actually parsed by Parsing an inner list is

inner-list = "(" OWS [ sh-item *( SP OWS sh-item ) [ SP OWS ] ] ")" *parameter

Your ABNF makes sense to me; I'd update the algorithm to match. Requiring OWS to begin with SP seems like a pretty arbitrary restriction.

phluid61 commented 5 years ago

Just noticed that the algorithms for token don't account for : or / – they only refer to tchar

mnot commented 5 years ago

yep, I had that queued up... :)

mnot commented 5 years ago
martinthomson commented 5 years ago

As I advocated for at the meeting, fixed-point decimal is far less likely to run into those issues. But apparently all that discussion at the meeting can be trumped by the fact that a floating point number has a larger dynamic range.

So seconds with nanosecond resolution needs 9 decimal places, but it can't do a large enough span of time to be useful as an absolute measure. I tend to think that that use case might be worth losing and that even 6 decimal places is a bit of a stretch. Or, if these are important enough, they can choose not to use SH.

phluid61 commented 5 years ago

Fixed-point decimal lacks the only advantage that floats have over integers, which is the dynamic range. If your header field doesn't need the dynamic range, use an integer – change it from 'seconds' to 'microseconds' if that's what's needed. (Also, can't do nanoseconds; precision starts at nine-point-six.)

WRT the name, as I mentioned elsewhere it might be valuable giving all the types names that don't map so closely to native types in common languages, to emphasise that SH types are actually different.

mnot commented 5 years ago

Suggestions for names welcome.

bsdphk commented 5 years ago

So seconds with nanosecond resolution needs 9 decimal places,

Donning my "Famous Faulty Predictins" hat, I will argue that there are not enough relevant needs for nano-seconds in HTTP headers to let that influence the design of SH.

I think caling the type "Decimal" instead of "Float" is not a bad idea.

mnot commented 5 years ago

Anyone against changing Float to Decimal?

mnot commented 5 years ago

Decimal now being discussed in #982.