Re: [PATCH 4/7] delta_base_cache: use list.h for LRU
- From
- Eric Wong <e@80x24.org>
- Date
- Aug 22, 2016, 23:18 UTC
- Message-ID
- <20160822231806.GA26148@starla>
- In-Reply-To
- <20160822215942.nm2jcrkyib7acr4u@sigill.intra.peff.net>
Jeff King <peff@peff.net> wrote:
Show 7 quoted lines
> We keep an LRU list of entries for when we need to drop > something from an over-full cache. The list is implemented > as a circular doubly-linked list, which is exactly what > list.h provides. We can save a few lines by using the list.h > macros and functions. More importantly, this makes the code > easier to follow, as the reader sees explicit concepts like > "list_add_tail()" instead of pointer manipulation.
Yay!
Show 6 quoted lines
> As a bonus, the list_entry() macro lets us place the lru > pointers anywhere inside the delta_base_cache_entry struct > (as opposed to just casting the pointer, which requires it > at the front of the struct). This will be useful in later > patches when we need to place other items at the front of > the struct (e.g., our hashmap implementation requires this).
On a side note, I think we should s/list_entry/container_of/ and use container_of for hashmap.
Linux kernel defines list_entry to use container_of, but I'd rather avoid introducing the duality entirely.
Show 13 quoted lines
> Signed-off-by: Jeff King <peff@peff.net> > --- > I think the result is much nicer, but I found list_entry() a little > disappointing, because we lack typeof(). So you are stuck writing: > > struct delta_base_cache_entry *f = > list_entry(p, struct delta_base_cache_entry, lru); > > I waffled on adding something like: > > LIST_ITEM(struct delta_bas_cache_entry, f, p, lru); > > to declare "f" as above. But it's getting rather magical and un-C-like.
Right. I'd rather keep the list_entry/container_of usage identical to what developers familiar using userspace-rcu, ccan/list, or the Linux kernel expect.
> sha1_file.c | 38 ++++++++++++++++---------------------- > 1 file changed, 16 insertions(+), 22 deletions(-)
Good to see the code reduction.