On Wed, Feb 19, 2025 at 08:41:03PM +0800, shejialuo wrote:
> But there is one thing I want to argue with. I don't think we need to
> rename "abort" callback to "release" and also "ref_iterator_abort" to
> "ref_iterator_free" for the following reasons:
>
> 1. We never call "release" expect in the "ref_iterator_free" function.
> For other exposed functions "ref_iterator_advance", "ref_iterator_peel"
> and the original "ref_iterator_abort". We will just call the registered
> callback "advance", "peel" or "abort" via virtual table. I somehow think
> we should follow this pattern. But I don't know actually.
> 2. When I read the patch yesterday, I really wonder what is the
> difference between "release" and "free". Why do we only change the
> "ref_iterator_abort" to "ref_iterator_free" but for the callback, we
> rename "abort" to "release". I know that you want to distinguish to
> emphasis that we won't free the iterator but only release its resource
> for ref iterator. But could abort also mean this?
The difference between "release" and "free" is explicitly documented in
our CodingGuidelines. Quoting the relevant parts:
- `S_release()` releases a structure's contents without freeing the
structure.
- `S_free()` releases a structure's contents and frees the
structure.
So following these coding guidelines, we have to call the underlying
implementations that are specific to the iterators `release()` because
they don't free the iterator itself. And because the generic part _does_
free the iterator itself in addition to releasing its state, it has to
be called `free()`.
Regarding the question why to even rename `ref_iterator_abort()` itself:
this is done to avoid confusion going forward. Previously it really only
had to be called when you actually wanted to abort an ongoing iteration
over its yielded references. This is not the case anymore, and now you
have to call it unconditionally after you're done with the iterator. So
while the naming previously made sense, now it doesn't anymore.
Patrick
