Re: [PATCH] protocol_t upgrade

On 03/01/2011 06:48 PM, Amos Jeffries wrote:
> On Mon, 28 Feb 2011 21:27:32 -0700, Alex Rousskov wrote:
>> On 02/28/2011 07:08 PM, Amos Jeffries wrote:
>>> On Mon, 28 Feb 2011 18:37:41 -0700, Alex Rousskov wrote:
>>>> On 02/28/2011 04:44 PM, Amos Jeffries wrote:
>>>>
>>>>>>> +/// Display the Protocol Type (in upper case).
>>>>>>> +inline std::ostream &
>>>>>>> +operator <<(std::ostream &os, ProtocolType const &p)
>>>>>>> +{
>>>>>>> + if (p < PROTO_UNKNOWN)
>>>>>>> + os << ProtocolType_str[p];
>>>>>>> + return os;
>>>>>>> +}
>>>>>>
>>>>>> Should we print something when p is PROTO_UNKNOWN or worse? Where
>>>>>> do we
>>>>>> use this operator?
>>>>>
>>>>> Any debugs dumping objects using the type. Potentially any streamed
>>>>> output of request/reply status lines.
>>>>>
>>>>> If the protocol is unset or unknown the ProtocolType value is not
>>>>> useful. The caller must do the output of any replacement data it has.
>>>>> Added documentation.
>>>>
>>>> If the operator is currently used for debugging only, I think we should
>>>> print invalid IDs, to increase the changes we can find the cause of the
>>>> ID-setting bug (and to make the debugging message shape consistent).
>>>>
>>>> If the operator is used for message formation, skipping invalid
>>>> protocol
>>>> IDs would lead to malformed messages, possibly aggravating the problem,
>>>> so an assert() or Must() may be more appropriate, but printing nothing
>>>> is probably worse than printing an ID.
>>>>
>>>> Not a big deal though and will probably go away (or change) if we
>>>> migrate towards an AnyP::Protocol class.
>>>
>>> Hmm, okay. Fair points.
>>>
>>> How does this work for you:
>>>
>>> if (p >= PROTO_NONE && p < PROTO_MAX)
>>> os << ProtocolType_str[p];
>>> else
>>> os << p;
>>>
>>> Leaving the compiler value in the stream for review when things go bad.
>>
>> Looks good to me!
>>
>> The following condition may look more natural to some:
>> (PROTO_NONE <= p && p < PROTO_MAX)
>
> I had to add a cast to prevent infinite inline loop :).
>
>
> Attached is an updated patch that I believe clears up all the issues we
> have discussed so far.

Thank you for polishing this! Here are two minor nitpicks that are not
worth a new patch revision:

> - return data->match (checklist->request->protocol);
> + return data->match(checklist->request->protocol);

Avoid unrelated whitespace changes if possible.

> + URLScheme sch = request->protocol; // temporary, until bug 1961 URL handling is fixed.

You can add "const" to these lines or just do the following in snprintf:
    URLScheme(req->protocol).const_str(),

I am not excited how URLScheme allows for implicit Protocol/URLSheme
conversions, but your patch does not introduce those conversions and
does not have to deal with them.

Thank you,

Alex.
Received on Wed Mar 02 2011 - 05:23:39 MST