Re: [PATCH v1] unit-tests for request first-line parser.

On 08/29/2010 05:09 PM, Amos Jeffries wrote:

>> * FooInit() is a C version of a Foo class constructor. I would recommend
>
>> converting HttpParserInit() to a constructor since you are polishing
>> this code anyway, but if you leave init() in place, please call clear()
>> _after_ performing any initializations in init(). This approach makes a
>> future conversion much easier.
>>
>
> Um, no.
>
> init() sets it to a state where data is available for parsing and about
> to begin.
>
> clear() sets it to a state where there is no data available and parsing
> is invalid.

For parsers, it is a bad idea to equate "no data available" with
"invalid", but, at any rate, the clear() method is misnamed: it is a
standard name in STL and other libraries and never implies invalidation,
just emptying and resetting. Your clear, if it must invalidate, should
be called invalidate() or something. And calling it from init() becomes
very confusing; please avoid that if clear() is actually invalidate().

> HttpParser is (ab)used in the current code by being re-used for multiple
> requests and responses *without clearing previous offset memory*.

What do you mean by "offset memory"? Does not init() resets offsets and
pointers?

> It has no
> buffers of its own, it merely points at the callers buffer space and hold
> offsets. Also, that callers buffer may at any time be re-allocated. Current
> code gets around this by re-init().

> Okay, for the basic fix+tests I'll remove clear() again. Leaving it and
> your comments to the polish stage.
> NOTE: we then cannot test for incorrect setting of supposedly unset
> fields.

Alex.
Received on Mon Aug 30 2010 - 01:22:10 MDT