On Thu, Feb 25, 2016 at 11:56 PM, Alex Rousskov <rouss...@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
sbuf-review.bundle
Description: Binary data
_______________________________________________ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev
