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

Alex Rousskov rousskov at measurement-factory.com
Thu Nov 19 06:03:38 UTC 2015

On 11/18/2015 01:03 AM, Amos Jeffries wrote:

> * 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.

If it was an error (and not just a warning), then that sounds like a
broken compiler to me. Said that, "apply specifiers consistently" is a
good general rule, of course. I ignored that rule to minimize
unnecessary old code changes.

I now modified UFSSwapDir.h, TestSwapDir.h, MemStore.h, and Transients.h
to comply with that rule per your request.

[ I think we would have to rely on compilers to find all the missed
cases. It is impractical to reliably do that manually because a class
may both introduce new virtual methods (no override) and override
parents virtual methods at the same time, making simple string searches

Also, because of various tricks like the CBDATA_CLASS() macro, we may
have to disable that compiler warning by default or in certain contexts.

Overall, enforcing consistent override use looks like a whole project on
its own! ]

> * 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?

I wish! That big problem is still there. It got a tiny bit easier to
solve because related non-Root methods have a simpler interface 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

That stub file is gone now. I discovered that it is no longer needed.

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

Committed to trunk (r14411).

Thank you,


More information about the squid-dev mailing list