[squid-dev] [PATCH] implement RFC3986

[Alex Rousskov]

On 03/15/2016 07:05 AM, Kinkie wrote:
>> I do not think so. The crux of the issue is that you are defending a
>> medium-size template function as a necessary and small evil, while I am
>> attacking the principle or direction of accommodating helper needs by
>> making Squid code worse.
> 
> Hi,
>   the attached patch is a modification of what was currently shared,
> de-templatized.
> It relies on SBuf and provides a convenient std::string-based wrapper,
> like you suggest.

Thank you.


> +extern const int16_t fromHexTable[256];
> +
> +/// \return the numeric representation of the HEXDIG argument ch, or -1 if invalid.
> +inline const int16_t
> +FromHex(unsigned char ch)
> +{
> +    // no need to check bounds, the lookup table has 256 entries
> +    return fromHexTable[ch];
> +}

AFAICT, FromHex() may be used before fromHexTable is initialized. Same
concern for the other 5+ global constants this patch introduces. AFAICT,
you have suffered from strange side effects of initialization order more
than enough! How about converting all these constants into
always-safe-to-use functions? If done right, there will be no difference
in performance of the proposed code AFAICT.


> +extern const CharacterSet
> +GenDelims,// RFC 3986 gen-delims set
> +SubDelims,// RFC 3986 sub-delims set
> +Reserved, // RFC 3986 reserved characters set
> +Unreserved, // RFC 3986 unreserved characters set
> +All;

One full/stand-alone declaration per line please.


> +    // XXX: SBuf lacking reserve(N)
> +    // rv.reserve(s.length()*2); //TODO: optimize arbitrary constant

AFAICT, SBuf::reserveSpace() should work fine here and in all other
define-SBuf-and-immediately-reserve-space contexts. Am I missing something?


Thank you,

Alex.