[squid-dev] [PATCH] libsbuf

[Kinkie]

On Thu, Feb 25, 2016 at 11:56 PM, Alex Rousskov
<rousskov at measurement-factory.com> wrote:
> On 02/25/2016 10:01 AM, Kinkie wrote:
>> Hi all,
>>   please find attached a the bundle implementing libsbuf (merge
>> proposal from lp:~squid/squid/sbuflab).
>>
>> It:
>> - shuffles SBuf core files into src/sbuf and implements sbuf/libsbuf.la
>> - refactors SBuf <-> String adaption functions into a separate header,
>> increasing decoupling
>> - reduces the list of stubs and libraries needed for SBuf unit tests
>> - removes _DEPENDENCIES clauses from affected unit tests' Makefile.am
>
> If possible, please do not call things foo/FooX.h. One foo is enough.

Sure. Preparing a rename patch.

> Is this v4.0-only change? Have you considered _not_ using a library (and
> a whole new directory) for these few SBuf-related files? Why not just
> include them in sources list using a Makefile variable?

My understanding is that the lib approach is the current recommended
best practice. Am I wrong?

> What does libsbuf.la depend on? If it depends on src/mem/ perhaps it
> should go there? Again, this SBuf-dedicated directory feels wrong (too
> narrow-scoped), and MemBlob should not really be there (if it is so
> narrowly scoped; what if I want MemBlob without SBuf)?

The shortest list of dependencies I can think of is in tests_testSBuf_SOURCES:
it requires full libbase, and at least stubs for time, debug, fatal and libmem.
This however raises a general question for all libraries: how do we
effectively document dependencies? Except for libbase, all are
expected to have some.

>> +#include "SBuf.h"
>
> You should #include SBuf.h the same way *everywhere*: sbuf/SBuf.h

Hadn't done so for files in src/sbuf/ . Fixed.

> These are not objections.
Please find attached the related merge bundle.

-- 
    Francesco
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sbuf-review.bundle
Type: application/octet-stream
Size: 19139 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20160229/e55cf305/attachment-0001.obj>