Re: [PATCH 0/1] Constify struct address_space_operations for 2.6.32-git-053fe57acv2

[Emese Revfy]

Pavel Machek wrote:
> Hi!
> 
>> Arjan van de Ven wrote:
>>> On Mon, 14 Dec 2009 22:25:26 +0100
>>> Pavel Machek <pavel@xxxxxx> wrote:
>>>
>>>> On Mon 2009-12-14 08:00:49, Arjan van de Ven wrote:
>>>>> On Mon, 14 Dec 2009 12:26:56 +0100
>>>>> Pavel Machek <pavel@xxxxxx> wrote:
>>>> I certainly object "constify ops... as much as possible". If it
>>>> uglifies the code, it should not be done. If it is as simple as adding
>>>> const to few lines, its probably ok.
>>>>
>>>> But .... the patch contained huge load of 
>>>>
>>>> -	int (* resume)()
>>>> +	int (* const resume)()
>>>>
>>>> What is that?
>>> the ops stuct instantiation itself should be const.
>>> the members not so much; that makes no sense.
>> Consitfying the structure fields prevents direct modifications of runtime
>> allocated ops structures therefore it gives a strong signal to the programmer
>> that he's trying to do something undesired (this approach is in fact already
>> used in the kernel, see: iwl_ops).
> 
> One const in structure declaration seems to be just enough, see:
> 
> const struct a {
> 	void (* f)(void);
> 	void (* const g)(void);
> } s;
> 
> void h(void)
> {
> 	struct a *p = &s;
> 	s.f = 0;
> 	s.g = 0;
> 	p->f = 0;
> 	p->g = 0;
> }
> 
> 
> delme.c: In function 'h':
> delme.c:8: warning: initialization discards qualifiers from pointer target type
> delme.c:9: error: assignment of read-only variable 's'
> delme.c:10: error: assignment of read-only variable 's'
> delme.c:12: error: assignment of read-only member 'g'
> 
> You get clean-enough warnings.
									Pave

Notice how you got an error for line 12 (p->g assignment) but no warning or error
at all for line 11 (p->f assignment). This example illustrates what I was explaining
so far:

if you dynamically allocate an ops structure (the result of which is a pointer type like
p in the above example) then with a non-const structure field you get no indication
from the compiler that you are doing something undesired, whereas with a const
structure field you get an error immediately. This is what helps a future developer
as it gives him a hint that he is doing something wrong and if he still insists on his
way of dynamic allocation, he will have to come up with ugly code
(e.g., void *(**)(void))(&p->g) = 0) that will not pass human review. You can teach gcc,
sparse, checkpatch, etc to recognize some of this ugliness but you cannot
programmatically detect all possible ways of evasion.
And if the compiler can help the developers, why not make use of it?

Note also that a const structure field helps the statically allocated non-const
variable case as well as the compiler will error out on such field modifications
(s.g assignment in my example) so the developer will again get a hint that he is
doing something undesired and will have to use direct initialisation (or write
the same ugly code as above that will not pass human review)
--
Emese

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