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

[Alex Rousskov]

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

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.

Done.


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

Alex.