[squid-dev] [PATCH] implement RFC3986

[Amos Jeffries]

Okay, I have done some of the major reimplementation changes and pushed
the result to lp:~squid/squid/rfc3986-mk2

I omitted the reserve() optimizations, but left in the SBuf::push_back
change.

The desired for-loop cannot be used apparently because SBuf lacks a
const_iterator definition. That will need to happen before we can apply
this, or put up with several very nasty const_cast.


The following audit changes have yet to be done:

On 29/12/2015 8:01 a.m., Kinkie wrote:
> On Mon, Dec 28, 2015 at 5:46 PM, Alex Rousskov wrote:
>> On 12/28/2015 07:01 AM, Kinkie wrote:
> 
>>> +static int
>>> +fromhex(char ch)
>>> +{
>>> +    if (ch >= '0' && ch <= '9')
>>> +        return ch - '0';
>>> +    if (ch >= 'a' && ch <= 'f')
>>> +        return ch - 'a' + 10;
>>> +    if (ch >= 'A' && ch <= 'F')
>>> +        return ch - 'A' + 10;
>>> +    return -1;
>>> +}
>>
>> If you are after performance, an unconditional lookup in a 256-member
>> int vector would be faster.
> 
> Performance is good, but it's not the primary objective. For now I'd
> leave as-is.
> 

>>> +// return a static 2-char buffer with a hex representation of argument
>>> +static char*
>>> +tohex(const unsigned char c)
>>> +{
>>> +    static char rv[3];
>>> +    (void) snprintf ( rv, 3, "%02X", c);
>>> +    return rv;
>>> +}
>>
>> If you are after performance, an unconditional lookup in a 256-member
>> char* vector would be much faster.
> 
> Same as above.
> 

>> If you can return a const pointer, please return a const pointer.
> 
> It's not possible with the current implementation. If we decide to
> move to an array-based one, sure.
> 

>>> +    for (auto in = s.begin(); in != e; ++in) {
>>> +        if (*in != '%') { // normal case, copy and continue
>>> +            rv.push_back(*in);
>>> +            continue;
>>> +        }
>> ...
>>
>> I cannot validate that low-level C code. Can you use a Tokenizer
>> instead? Is it critical to provide copy-less performance for helpers
>> using std::string instead of SBuf?
> 
> Performance for helpers is not really critical IMO.
> SBuf is a lot of code and it really depends on infrastructure provided
> by squid (e.g. mempools).
> Outside squid it really makes not much sense to bring in that much
> scaffolding - which would also have include stubs etc.
> It's much better to be API-compatible and use std::string there.
> 

>>> +public:
>>> +    const static CharacterSet
>>> +        Unsafe,  // RFC 1738 unsafe set
>>> +        Ctrls,   // control characters (\0x00 to \0x1f)
>>> +        UnsafeAndCtrls, // RFC 1738 Unsafe and Ctrls
>>> +        Reserved1738, // RFC 1738 Reserved set
>>> +        GenDelims,// RFC 3986 gen-delims set
>>> +        SubDelims,// RFC 3986 sub-delims set
>>> +        Reserved, // RFC 3986 reserved characters set
>>> +        Unreserved, // RFC 3986 unreserved characters set
>>> +        Unescaped,//ctrls and unsafe except for percent symbol
>>> +        All;
>>> +
>>> +};
>>
>> This is not a class but a namespace.
>>
>> Please s/RFC3986/Rfc3986/ for consistency with similar "let's group RFC
>> concepts in one namespace" SslBump code posted by Christos earlier (IIRC).
>>

I was not able to find that posting yet. Have done the split into two
namespaces anyway. But would like to check against Factory ideas anyway
to make sure we are not diverging implementation style.

Amos