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. _______________________________________________ squid-dev mailing list [email protected] http://lists.squid-cache.org/listinfo/squid-dev
