[squid-dev] [PATCH] Store API changes blocking bug #7

[Amos Jeffries]

On 18/11/2015 7:31 p.m., Alex Rousskov wrote:
> Hello,
> 
>     This first step towards bug #7 fix focuses on fixing several Store
> API problems and polishing Store code. Many Store problems remain to be
> solved, but this is a significant step forward IMO, even though no
> functionality changes were intended during this step. The next steps
> will address bug #7 itself.
> 

Thank you. Audit below. Since I'm still not completely familiar with
Store I've not tried to do any deep analysis. It does appear to be
largely cosmetic though.


* in src/fs/ufs/UFSSwapDir.h and src/tests/TestSwapDir.h (at least) you
have one class with a mix of override/non-override virtuals.
 - Kinkie and I have found the hard way earlier that some compilers
error if only some virtual methods in a class heirarchy are marked with
it. It is an all-or-nothing per-heirarchy situation with that keyword.
 - please coose whether to use it or not, and update appropriately
before this goes in.


* I seem to recall a fair amount of trouble over the local vs non-local
vs unused/dead objects situation on a per-object basis rather than
per-store. That was behind wantsLocalMemory parameter to
Store::Controller::dereferenceIdle() which is being removed from some of
the sub-calls.
 - is that a fixed problem now?


* in src/store/Disks.cc please take advantage of the move to doxygen
commen (\**) the functions that have prefix documentation already.


* in src/tests/stub_store_search.cc please use nullptr
 - also since the stub is for StoreSearch.cc it should be named
stub_StoreSearch.cc


+1 provided the override issue is fixed. Though I don't think that will
need another audit.


Amos