Message-ID: <70318cbf0910112218j50a63148t2e044bc5451dc0cb@mail.gmail.com>
Date:	Sun, 11 Oct 2009 22:18:24 -0700
From:	Christopher Li <sparse@...isli.org>
To:	Josh Triplett <josh@...htriplett.org>
Cc:	linux-sparse@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] New attribute designated_init: mark a struct as requiring 
	designated init

On Sat, Oct 10, 2009 at 9:03 AM, Josh Triplett <josh@...htriplett.org> wrote:
> After getting this patch reviewed, I intended to add many such
> annotations myself.  I have patches sitting in my local Linux tree.
> (However, I don't plan to try to get the patches accepted into Linux
> until this attribute shows up in a Sparse release, since I don't want to
> make Linux require the latest Sparse from the git tree.)

I apply your patch so you can have a fair chance to pitch to the
kernel. If nobody use it, I can remove it later. I don't think you need
to wait for the patch to go into sparse release to collect feed back
from lkml.


> You *hope* that positional initialization will fail. :)  Some of these
> types of structures contain many fields with the same or similar types,
> and initializers don't inherently contain a lot of distinguishing type
> information that helps cause type errors rather than silent failures.

I am very interested to know some precedence from the kernel commit history.
If we can find such a commit which changes the structure and breaks
other initialization due to the position initialization, it will make your
case stronger as well.

> Even if the positional initialization does fail, this failure will occur
> at the time of attempting to change the structure, and many such
> failures may occur all over the tree, making such changes significantly
> more difficult.  With the designated_init attribute, sparse will warn
> when attempting to create the structure initializer.

Without sparse, you can temporary  insert some unused strange type
into the beginning of the struct. That should cause any position initializer
fail to compile. You can then find call the call site.

> With the designated_init attribute, someone making changes to a
> structure can check that Sparse produces no warnings about positional
> initializers and then know that certain types of changes can occur
> without having to check every initializer.  For instance, removing a
> field will only require checking any initializer that initializes that
> field, and the compiler will find all of those.  Adding a field that can
> safely remain NULL does not require checking any initializers.

I think the initializer is just a small part of the work if you even change
the structure. You need to make sure the *code* use these C struct
actually works as expected.

> (As an aside, though: any fundamental reason why we use that error-prone
> approach rather than allocating one extra byte and NUL-terminating
> idents?  Then we could drop the (larger) len field, and remove the
> annoying restriction on calling show_ident multiple times in the same
> statement.)

When sparse does the hash table look up, if there is collision on the
hash table entry, sparse will compare the size first before the memcmp().
That will reduce the number of the memcmp call it makes. I would like to
keep the size of struct ident. If any thing, we can NUL terminate the string
in ident to make printing easy.

Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists