Messages in this thread | | | Date | Wed, 16 Dec 2009 00:28:36 +0100 | From | Emese Revfy <> | Subject | Re: [PATCH 0/1] Constify struct address_space_operations for 2.6.32-git-053fe57ac v2 |
| |
Pavel Machek wrote: > Hi! > >> Arjan van de Ven wrote: >>> On Mon, 14 Dec 2009 22:25:26 +0100 >>> Pavel Machek <pavel@ucw.cz> 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@ucw.cz> 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
| |