[squid-dev] [PATCH] Store API changes blocking bug #7
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
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).
More information about the squid-dev