[squid-dev] [PATCH] PackableStream for cachemgr reports

[Alex Rousskov]

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".


> +    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.


> +            // 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.

Make "chars" constant if you can.


> +                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.


> +        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.


> +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.


> +    PackableStream(Packable *entry): std::ostream(0), theBuffer(entry) {

Missing "explicit" or a comment justifying implicit conversions.

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.


> +    /* create a stream for writing text etc into theEntry */

No theEntry here.


> +    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).


> +    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.

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?


> * 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?


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 &);
>                   ^


HTH,

Alex.