Re: [squid-dev] [PATCH] libsbuf

[Kinkie]

On Thu, Feb 25, 2016 at 11:56 PM, Alex Rousskov
 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

Re: [squid-dev] [PATCH] libsbuf

[Kinkie]

On Thu, Feb 25, 2016 at 6:50 PM, Amos Jeffries  wrote:
> On 26/02/2016 6:01 a.m., 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
>>
>> It has to be a bundle to carry information about file moves; this
>> means no long-context diff, I'm sorry.
>> Code builds fine in-tree. I'll perform a full test before possibly
>> merging if you approve the patch, of course.
>>
>
> I think all the stats.allocFromString accounting could stay. It would
> just be for std::string now instead of combined std::string and SquidString.
>
> OR, maybe the String to/from counting could be done by the
> SBufStringConvert.h functions instead of the SBuf.

As part of my discussion with Alex we talked about reducing the number
of statistics taken. I believe that number not to be  very relevant
and thus removed it. Other such statistics will follow

> in src/Makefile.am:
> * tests_testSBuf_LDADD needs to include $(SQUID_CPPUNIT_LA)
>  - same for tests_testSBufList_LDADD and tests_testLookupTable_LDADD

Done.

> in src/mk-string-arrays.awk:
> * can you take advantage of the edit to s/namesapce/namespace/ please?

Done

> in src/sbuf/SBufStringConvert.h:
> * use:  #include "sbuf/SBuf.h"

ok

> in src/tests/testSBuf.cc:
> * use: #include "tests/SBufFindTest.h"

Done.


-- 
Francesco
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] libsbuf

[Amos Jeffries]

On 26/02/2016 6:01 a.m., 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
> 
> It has to be a bundle to carry information about file moves; this
> means no long-context diff, I'm sorry.
> Code builds fine in-tree. I'll perform a full test before possibly
> merging if you approve the patch, of course.
> 

I think all the stats.allocFromString accounting could stay. It would
just be for std::string now instead of combined std::string and SquidString.

OR, maybe the String to/from counting could be done by the
SBufStringConvert.h functions instead of the SBuf.


in src/Makefile.am:
* tests_testSBuf_LDADD needs to include $(SQUID_CPPUNIT_LA)
 - same for tests_testSBufList_LDADD and tests_testLookupTable_LDADD


in src/mk-string-arrays.awk:
* can you take advantage of the edit to s/namesapce/namespace/ please?


in src/sbuf/SBufStringConvert.h:
* use:  #include "sbuf/SBuf.h"


in src/tests/testSBuf.cc:
* use: #include "tests/SBufFindTest.h"


Amos

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

[squid-dev] [PATCH] libsbuf

[Kinkie]

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

It has to be a bundle to carry information about file moves; this
means no long-context diff, I'm sorry.
Code builds fine in-tree. I'll perform a full test before possibly
merging if you approve the patch, of course.

Thanks

-- 
Francesco


sbuflib.bundle
Description: Binary data
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev