On 8/08/2015 5:04 p.m., Alex Rousskov wrote: > On 08/05/2015 10:24 AM, Amos Jeffries wrote: >> Adds a PackableStream class which provides std::ostream semantics for >> writing data to a cachemgr report. > > >> + PackableStreamBuf(Packable *out) : outBuf(out) {} > > Missing "explicit" or a comment justifying implicit conversions. > > >> + ~PackableStreamBuf() {} > > Please add "virtual". >
Fixed both. Darn cut-n-paste. Also fixing the StoreEntryStream.h where these originated. > >> + virtual int_type overflow(int_type aChar = traits_type::eof()) { > >> + virtual int sync() { > >> + virtual std::streamsize xsputn(const char * chars, std::streamsize >> number) { > > Please add "override" to each overridden virtual method from the > std::streambuf API. I dont quite follow you here. Is that a request to add comments? or an attribute I was not aware of? > >> + // NP: cast because GCC promotes int_type to 32-bit type >> + // std::basic_streambuf<char>::int_type {aka int} >> + // despite the definition with 8-bit type value. >> + char chars[1] = {char(aChar)}; > > Please use static_cast (or another appropriately named cast) to cast. > > Do not declare "chars" when you do not need them. The comment says 'cast' because that is essentially what is being done. However the char(int_type) type constructor is necessary because this is the fundamental int_type being passed in. All the available *_cast<char>() operators are built to convert a 32-bit int_type value into an array of 4 bytes before dealing with the byte->char conversion part. Which then incorrectly takes the lowest-8 bits rather than the first-8. The int_type being passed in actually has only 8-bits of length, not 32-bits. Which screws over those cast operators memory accesses. They typically end up casting the value at 2nd or 4th byte of a 1-byte stack object to char. If you are aware of a foo_cast<>() operator that correctly converts in the presence of that traits weirdness I am interested. > > Make "chars" constant if you can. Trying. > >> + outBuf->append(chars, 1); > ... >> + outBuf->append(pbase(), pending); > ... >> + outBuf->append(chars, number); > > To reduce code duplication and to assist with debugging, it would be > nice to move these three into a single dedicated private write() method > or similar. > Calling xsputn() instead now. Although note that trampolining off a virtual function (non-inlinable in some compilers) actually makes it less efficient. > >> + if (pending) >> + outBuf->append(pbase(), pending); > ... >> + if (number) >> + outBuf->append(chars, number); > > Are you sure you are saving more cycles by bypassing zero-size appends > than you are wasting on the size check? If you are not, do not waste > cycles and ink on that check. The MemBuf and SBuf implementations of append() check for 0 themselves. But the StoreEntry assumed non-0 and does some relatively heavy allocation. PackableStream(StoreEntry) is expected to be the common use case so this is a net performance gain until StoreEntry itself gets optimized. > >> +private: >> + Packable *outBuf; >> +}; > > Why is this a pointer? The class code seems to require a non-null > pointer. If you can make this a reference, please do. > > If this member remains a pointer, we ought to check that it is not null. It was pointer in StoreEntyStream. Trying reference. > >> + PackableStream(Packable *entry): std::ostream(0), theBuffer(entry) { > > Missing "explicit" or a comment justifying implicit conversions. Fixed. > > If you do not want this class to be responsible for checking "entry" for > being nil (after the PackableStreamBuf fixes above), then use a > reference instead of a pointer. > > Please s/entry/packable/ or something like that. We should not imply > that this is StoreEntry even if it [usually] is today. Nod. Using p like everywhere else. > > >> + /* create a stream for writing text etc into theEntry */ > > No theEntry here. Fixed. >> + virtual void dump(std::ostream &) {} > > This should either throw (if it must not be called unless overridden) or > provide some debugging (if it is OK to leave it a no-op). > It is okay to be a no-op. It should not be called if not overridden. Making it dump "X is NOT Implemented" to the stream for now. > >> + virtual void dump(std::ostream &) {} >> + >> + // magic wrapper. >> + // old code will override this instead of the new ostream dumper. >> + virtual void dump(StoreEntry *e); > > IIRC, it is not possible to override just one dump() without hiding the > other. I do not know whether we enable compiler warnings about hiding > virtual methods by default, but we _should_ enable them if we do not > already. I thought both symbols had a vtable entry under their different mangling. Running some extra tests to check that now. > > Perhaps more importantly, why do we actually need this [black] magic > here? Can the new method just use a different name, like dumpToStream(), > until the transition is over? The only black magic bit is if both virtuals get hidden when one is overridden. And that is anti-useful. So if it happens I will definitely rename. Otherwise it is six of one, half a dozen of the other. > >> * convert old Action classes to new API >> * refactor old C-code report generators to be Action classes >> - fixing display output syntax to minimal YAML as we go on both the >> above. It mostly is already, but some reports have wrong syntax. > > It feels wrong that you want Actions to output using strict syntax but > give them an "free form" or "anything goes" std::ostream as a tool to > accomplish that. Should not Action dumping methods receive a specialized > class that makes it easy to produce the output you want and hard (if not > impossible) to make syntax mistakes? > Action itself is currently that class you speak about AFAICT. Maybe in future the "update Action API to receive client desired format" will be a formatter class rather than just header value. For now its just a free-form mix in plain text. To which a stream is suited. And possible something the formatter classes could use better than a direct Packable object pointer/reference. Given the 4 years it has taken to get this far I'm expecting the stream and cut-down YAML will be used for a long time before we get around to debating better formatter syntax(es) again. > > Finally, "make check" fails in my tests with several errors like: > > >> tests/stub_libmgr.cc:65:6: error: prototype for ‘void >> Mgr::RotateAction::dump(StoreEntry*)’ does not match any in class >> ‘Mgr::RotateAction’ >> void Mgr::RotateAction::dump(StoreEntry *entry) STUB >> ^ >> In file included from tests/stub_libmgr.cc:48:0: >> ../src/mgr/BasicActions.h:78:18: error: candidate is: virtual void >> Mgr::RotateAction::dump(std::ostream&) >> virtual void dump(std::ostream &); >> ^ > Fixing. Followup patch to come when the virtual experiments and re-tests are completed. Amos _______________________________________________ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev