Re: [PATCH] protocol_t upgrade

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)

Thank you,

Alex.
Received on Tue Mar 01 2011 - 04:27:44 MST