Re: [PATCH v3] kvm tools: Add QCOW level2 caching support

[Ingo Molnar <mingo@elte.hu>]

* Pekka Enberg <penberg@kernel.org> wrote:

> On Fri, Jun 3, 2011 at 12:19 AM, Sasha Levin <levinsasha928@gmail.com> wrote:
> >> +static int cache_table(struct qcow *q, u64 *table, u64 offset)
> >> +{
> >> +     struct qcow_l2_cache *n;
> >> +     struct rb_root *r = &q->root;
> >> +     struct qcow_l2_cache *lru;
> >> +
> >> +     n = calloc(1, sizeof(struct qcow_l2_cache));
> >                        sizeof(*n)
> > sizeof() should use the variable name itself, not the data type. Check
> > out chapter 14 in 'Documentation/CodingStyle'.
> 
> Well, it doesn't matter that much, to be honest. 'n' could use a 
> better name, though - 'cache' or 'c'.

I personally prefer the sizeof(*cache) variant for a subtle reason: 
because during review it's easier to match up local variable names 
than to match up types.

For example when i review code i only look at the types once: i just 
establish their main nature and attach any semantic meaning to the 
local variable name.

So if 'later in the code' i see "sizeof(struct qcow_l2_cache)" i wont 
know it intuitively whether it matches up with 'cache' or not. So for 
example i might not notice such a bug:

	cache = calloc(1, sizeof(struct qcow_l1_cache));

(trivia: how many seconds does it take for you to notice the bug in 
the above line? Note, you already have the advantage that you *know* 
that there's a bug in that line :-)

Even if i noticed the bug, i'd notice it by looking back at the local 
variables defininition block - which is a distraction from reading 
the code flow itself.

But if it's written differently i will notice this bug immediately:

	cache = calloc(1, sizeof(*r));

i will even notice this pattern:

	cache = calloc(1, sizeof(cache));

I guess we could introduce a type-safe zalloc variant, something 
like:

	cache = zalloc_t(*cache);

Which is a clever macro that allocates sizeof(struct qcow_l2_cache) 
bytes and gives back a 'struct qcow_l2_cache *' typed result. That 
way this:

	cache = zalloc_t(r);

or this:

	cache = zalloc_t(cache);

Will result in a compiler error.

Hm?

Thanks,

	Ingo