From: Chandra Pratap Date: Mon, 06 Jul 2026 15:33:12 GMT Subject: Re: [PATCH v7 2/3] graph: add a 2 commit buffer for lookahead Message-ID: In-Reply-To: On Mon, 6 Jul 2026 at 19:15, Kristofer Karlsson wrote: > > The hardcoded size-2 lookahead buffer was my suggestion, > so I am responding inline with my thoughts although Pablo is > the right person for making further changes (if any). > > On Mon, 6 Jul 2026, Chandra Pratap wrote: > > Do we need to NULL out the retrieved buffer entries? If so, it is > > worthwhile asserting that the entire buffer is NULLed out in the > > !graph->lookahead_nr check above. > > You're right, it's not technically needed, and there are many places > in the repo where stale data remains in buffers, and it would be possible > to do that here too. I don't think it matters much in practice though, > and NULLing them out would perhaps prevent some accidental reuse on bugs > (NULL would crash instead). > > As for asserting: rather than checking that empty slots are NULL > (which just verifies our own cleanup), it might be more useful to > assert that a slot is non-NULL when lookahead_nr says it should be > populated, i.e. assert on read rather than on empty. But even that > may be overkill for a 2-element internal buffer. True. But since we're already going through the pains of initializing the buffer and NULLing it upon a pop, I'd much rather go the extra length and verify what we're trying to do, shouldn't be that complicated anyway. Whether that means checking for NULL here, on a push, or on a read is something I don't feel strongly about, either is fine with me. > > Not the best engineering practice, but I guess it is fine to constrain > > the logic to _only_ a 2-entry buffer since that's what we'll always > > deal with anyway. > > I did consider making it a proper ring buffer, but it felt like > overkill (and I could not find any other existing ring buffer to > piggy-back on in the repo), and the lookahead depth is > structurally tied to the algorithm - we only ever need two more > elements. > > It also helps that this is entirely internal to graph.c. If the > buffer were part of a broader API, a less hardcoded approach > would be more appropriate indeed. Agreed. > > We should use ARRAY_SIZE(graph->lookahead) instead of hardcoding > > the value 2. > > Agreed, that is a nice improvement. What do you think Pablo? > > Thanks, > Kristofer