On Sun, 29 Aug 2010 19:22:02 -0600, Alex Rousskov <[email protected]> wrote: > 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? By "offset memory" I mean the values from previous runs that HttpParser stores for offset to various pieces of the request line. init() reset the overall start/end offsets known for the line, but left the finer details like start/end offsets in held for method/URL/version at some random value from a previous run or in the case of a unused HttpParser, pointing at some random offset in RAM heap space relative to the about to be processed read buffer. > >> 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.
