* Pekka Enberg <penberg@xxxxxxxxxx> wrote:
> On Fri, Jun 3, 2011 at 12:19 AM, Sasha Levin <levinsasha928@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
