Maintenance: update FadingCounter API #1782
Conversation
This value is never changed, so it does not need to be a modifiable member. Also, update type to size_t to avoid casting.
This value is only ever used for whole integers of seconds. It does not need the millisecond resolution. Hide fully behind accessor methods that preserve the -1/0/+N semantics of the existing API.
Co-authored-by: Francesco Chemolli <5175948+kinkie@users.noreply.github.com>
There was a problem hiding this comment.
AFAICT, this PR introduces runtime bugs, but I am focusing this review on primary API changes: If my horizon type recommendations are followed, those bugs may go away automagically.
Please test FadingCounter configurations with -1, 0, and 1 second horizon parameters before requesting additional reviews.
| FadingCounter(); | ||
|
|
||
| /// 0=remember nothing; -1=forget nothing; new value triggers clear() | ||
| void configure(double horizonSeconds); | ||
| void configure(const time_t horizonSeconds); |
There was a problem hiding this comment.
If we are going to change FadingCounter::horizon type, then we should use an std::chrono::duration type.
N.B. I am aware of "This value is only ever used for whole integers of seconds." fact and "It does not need the millisecond resolution." assertion in PR commit e4847b5 message, but neither convince me that limiting future configure() callers to integer numbers is a desirable change because I can imagine callers that would be inconvenienced by it (e.g., callers that have to compute horizon value by dividing some configuration parameters). I do not plan to block this PR because of this added restriction, but I recommend keeping support for fractional seconds.
There was a problem hiding this comment.
One step at a time. Using a std::chrono here requires squid.conf parser logic and modifications to the internal logic of FadingCounter. They are explicitly being left to followup PR.
There was a problem hiding this comment.
Using a
std::chronohere requiressquid.confparser logic
I seriously doubt that using std::chrono for FadingCounter::horizon requires any squid.conf parser changes, but, if it really does, then FadingCounter::horizon type and similar changes are not compatible with an "Update FadingCounter API" PR (because they require too many out-of-scope changes).
and modifications to the internal logic of
FadingCounter.
I do not think so, but our definitions of "internal logic" may differ.
They are explicitly being left to followup PR.
In summary, it is totally fine to postpone conversion of FadingCounter internals to std::chrono beyond this PR as long as this PR does not change FadingCounter::horizon type (or contains other similar internal FadingCounter changes that are difficult to do correctly, especially without std::chrono help).
There was a problem hiding this comment.
FadingCounter::horizon is set by several configuration options. Currently they use parse_time(). I do not want to extend scope of this specific PR outside of FadingCounter.
There was a problem hiding this comment.
FadingCounter::horizonis set by several configuration options. Currently they useparse_time(). I do not want to extend scope of this specific PR outside ofFadingCounter.
No such scope extension is requested, required, or implied by this change request. FadingCounter::horizon is configured by FadingCounter::configure(double). One can keep that configuration API intact while converting FadingCounter::horizon to std::chrono::duration. That std::chrono conversion itself is only required if this PR changes FadingCounter::horizon type.
Please add the missing unit-tests for |
Personally, I do not plan on adding unit-tests for FadingCounter (and I am not sure I agree that they are "missing").
This request is not about existing official code. I strongly suspect that the current PR introduces new bugs in FadingCounter implementation. If my suspicions are correct, then I hope that the requested tests expose those bugs (and prevent them from re-occurring as we refactor PR code to address change requests). I am not asking for anything complex or permanent like official unit tests. I do not even need to see your test code. |
| { | ||
| for (int i = 0; i < precision; ++i) | ||
| counters[i] = 0; | ||
| std::fill(counters.begin(), counters.end(), 0); |
There was a problem hiding this comment.
Please simplify this code, similar to code this PR already uses to initialize counters during object construction:
| std::fill(counters.begin(), counters.end(), 0); | |
| counters = {}; |
| total -= counters[idx]; | ||
| counters[idx] = 0; | ||
| Must(total >= 0); |
There was a problem hiding this comment.
This assertion can be preserved if it is moved before total is decremented and adjusted to look like Assure(total >= counters[idx]).
There was a problem hiding this comment.
I removed this and the one below because they seem to exist purely for paranoia reasons and do not appear to be necessary any longer.
rousskov
added
the
S-waiting-for-author
No description provided.