[squid-dev] [PATCH] renaming StoreEntryStream to PackableStream

[Alex Rousskov]

On 08/21/2015 04:38 AM, Amos Jeffries wrote:
> So PackableStream is really just a rename of StoreEntryStream BUT with
> some implicit new properties from the underlying type change:
> 
> * lack of Store.h dependency
> 
> * ability to stream into a MemBuf if its creator desires that
> 
> This patch demonstrates that by doing the upgrade in-place and re-typing
> all StoreEntryStream objects.
> 
> It includes some extra polishing of the *StreamBuf logics to remove
> styling and redundant code issues identified in the previous cycles of
> discussion. But otherwise is essentially a drop-in replacement.


Thank you for making a well-scoped change. It makes finding problems a
lot easier.


> +    virtual int sync() override {
>          std::streamsize pending(pptr() - pbase());
> -
> -        if (pending)
> -            theEntry->append(pbase(), pending);
> -
> -        theEntry->flush();
> -
> +        lowAppend(pbase(), pending);
>          return 0;
>      }

Patched sync() no longer flush()es the entry. This is a significant
undocumented change and, AFAICT, a major bug. If I am right (see below
for specifics), please consider adding a pure virtual Packable::flush()
method and calling that in the above code _regardless_ of the "pending"
value, just like the old code used to do.


> +    PackableStream stream(*sentry);
...
>      stream.flush();

How would that flush() call reach "sentry" if PackableStreamBuf never
flushes its buf_?


> +        anEntry->lock("test");
> +        PackableStream stream(*anEntry);
>  
>          stream.setf(std::ios::fixed);
>          stream << 123456 << std::setprecision(1) << 77.7;
>          stream << " some text" << std::setw(4) << "!" << '.';
>          stream.flush();
> -        // flushed at least once more
> -        CPPUNIT_ASSERT(anEntry->_flush_calls > preFlushCount);
> +        CPPUNIT_ASSERT_EQUAL(String("12345677.7 some text   !."), anEntry->_appended_text);
>  
> +        delete anEntry; // does the unlock()

Checking the appended text without checking that the entry was flushed
is not sufficient because the appended text does not know what store
entry sent to Store clients. It only shows what Store servers appended
to the entry.


> +        anEntry->lock("test");
> +        PackableStream stream(*anEntry);
...
> +        delete anEntry; // does the unlock()

This leads to a dangling reference in the "stream" object. Please
consider narrowing the "stream" scope so that it is destroyed before you
delete anEntry.


> +    PackableStreamBuf(Packable &p) : buf_(p) {}

If you can make the ctor explicit, please do.


> +    PackableStream(Packable &p) : std::ostream(0), theBuffer(p) {

Ditto.


> +    ~PackableStreamBuf() = default;

I would remove that for simplicity and consistency sake.

Rule of thumb: Given a set of method kinds that can be marked as default
or can be explicitly deleted (i.e., copy constructor, destructor,
assignment operator, etc.), we should either explicitly declare all such
methods or none at all. None is the right choice when we do not _need_
to redefine any default methods or explicitly delete any methods.

The "none" choice applies to both PackableStreamBuf and PackableStream.


HTH,

Alex.