Hi,
On Sun, Apr 03, 2022 at 01:24:40PM +0300, Andrei Zubkov wrote:
>
> On Sun, 2022-04-03 at 17:56 +0800, Julien Rouhaud wrote:
> > Just another minor nitpicking after a quick look:
> >
> > + This field will be zero if ...
> > [...]
> > + this field will contain zero until this statement ...
> >
> > The wording should be consistent, so either "will be zero" or "will
> > contain
> > zero" everywhere. I'm personally fine with any, but maybe a native
> > English
> > will think one is better.
> Agreed.
>
> Searching the docs I've fond out that "will contain" usually used with
> the description of contained structure rather then a simple value. So
> I'll use a "will be zero" in the next version after your review.
Ok!
So last round of review.
- the commit message:
It should probably mention the mimnax_stats_since at the beginning. Also, both
the view and the function contain those new field.
Minor rephrasing:
s/evicted and returned back/evicted and stored again/?
s/with except of all/with the exception of all/
s/is now returns/now returns/
- code:
+#define SINGLE_ENTRY_RESET() \
+if (entry) { \
[...]
It's not great to rely on caller context too much. I think it would be better
to pass at least the entry as a parameter (maybe e?) to the macro for more
clarity. I'm fine with keeping minmax_only, stats_reset and num_remove as is.
Apart from that I think this is ready!
