Thanks for reviewing.
On Thu, Apr 03, 2025 at 03:23:40PM -0700, Jeff Davis wrote:
> This simplifies commit a0a4601765. I'd break out that simplification as
> a separate commit to make it easier to understand what happened.
Done.
> In patch 0003, there are quite a few static function-scoped variables,
> which is not a style that I'm used to. One idea is to bundle them into
> a struct representing the cache state (including enough information to
> fetch the next batch), and have a single static variable that points to
> that.
As discussed off-list, I didn't take this suggestion for now. Corey did
this originally, and I converted it to static function-scoped variables 1)
to reduce patch size and 2) because I noticed that each of the state
variables were only needed in one function. I agree that a struct might be
slightly more readable, but we can always change this in the future if
desired.
> Also in 0003, the "next_te" variable is a bit confusing, because it's
> actually the last TocEntry, until it's advanced to point to the current
> one.
I've renamed it to expected_te.
> Other than that, looks good to me.
Great. I'm planning to commit the attached patch set tomorrow morning.
For the record, I spent most of today trying very hard to fix the layering
violations in 0002. While I was successful, the result was awkward,
complicated, and nigh unreadable. This is now the second time I've
attempted to fix this and have felt the result was worse than where I
started. So, I added extremely descriptive comments instead. I'm hoping
that it will be possible to clean this up with some additional work in v19.
I have a few ideas, but if anyone has suggestions, I'm all ears.
--
nathan
