Alexander, On Tue, 26 Dec 2023 at 23:35, Alexander Korotkov <aekorot...@gmail.com> wrote:
> Pavel, > > On Mon, Dec 25, 2023 at 8:32 PM Pavel Borisov <pashkin.e...@gmail.com> > wrote: > > I've reviewed both patches: > > 0001 - is a pure refactoring replacing argument transfer from via struct > member to transfer explicitly as a function argument. It's justified by the > fact firstPage is localized only to several places. The patch looks simple > and good enough. > > > > 0002: > > continuescanPrechecked is semantically much better than previous > requiredMatchedByPrecheck which confused me earlier. Thanks! > > > > From the new comments, it looks a little bit hard to understand who does > what. Semantics "if caller told" in comments looks more clear to me. Could > you especially give attention to the comments: > > > > "If they wouldn't be matched, then the *continuescan flag would be set > for the current item and the last item on the page accordingly." > > "If the key is required for the opposite direction scan, we need to know > there was already at least one matching item on the page. For those keys." > > > > > Prechecking the value of the continuescan flag for the last item on the > > >+ * page (according to the scan direction). > > Maybe, in this case, it would be more clear like: "...(for backwards > scan it will be the first item on a page)" > > > > Otherwise the patch 0002 looks like a good fix for the bug to be pushed. > > Thank you for your review. I've revised comments to meet your suggestions. > Thank you for revised comments! I think they are good enough. Regards, Pavel
