Hi,
On Thu, 27 Mar 2025 at 03:48, Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> On Mon, Mar 17, 2025 at 9:47 AM Matheus Alcantara
> <matheusssilv97@gmail.com> wrote:
> >
> > Sorry for the delay, attached v4 with the remaining fixes.
>
> Thanks for the patch.
>
> I started reviewing this with the intent to commit it. But, I decided
> while studying it that I want to separate the SKIP_PAGES_NONE case and
> the other cases into two callbacks. I think it is easier to read the
> skip pages callback this way. The SKIP_PAGES_NONE case is just read
> all blocks in the range, so we can use the existing default callback,
> block_range_read_cb(). Then the callback for the
> SKIP_PAGES_ALL_VISIBLE and SKIP_PAGES_ALL_FROZEN options can be clear
> and simple.
>
> I've attached two versions with this proposed structure.
>
> amcheck-readsteram-1callback.patch implements this with one callback
> and has the amcheck specific callback private data struct subclass
> BlockRangeReadStreamPrivate (I called it
> heapamcheck_rs_perblock_data).
>
> amcheck-readstream-2callbacks.patch wraps block_range_read_cb() in an
> amcheck specific callback and creates a BlockRangeReadStreamPrivate
> and fills it in from the heapamcheck_rs_perblock_data to pass as
> callback_private_data. Because this version is more explicit, it is
> more safe. We don't have any type checking facilities that will alert
> us if someone adds a member above the BlockRangeReadStreamPrivate in
> heapamcheck_rs_perblock_data. But, I'm open to feedback.
I liked the first approach more. We can solve the first approach's
problems by introducing a void pointer to pass to
read_stream_begin_relation(). We can set it to &rsdata.range for the
SKIP_PAGES_NONE case and &rsdata for the rest.
Example patch is attached, heapamcheck_rs_perblock_data is added to
typedefs.list too.
--
Regards,
Nazir Bilal Yavuz
Microsoft
