Date: Sun, 17 May 2009 14:36:03 +0200 From: Christoph Mallon <christoph.mallon@gmx.de> To: Stanislav Sedov <stas@FreeBSD.org> Cc: FreeBSD Hackers <freebsd-hackers@freebsd.org> Subject: Re: C99: Suggestions for style(9) Message-ID: <4A1004B3.5040805@gmx.de> In-Reply-To: <20090517144516.331b01a8.stas@FreeBSD.org> References: <49F4070C.2000108@gmx.de> <20090428114754.GB89235@server.vk2pj.dyndns.org> <49FAE4EA.1010205@gmx.de> <20090517144516.331b01a8.stas@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Stanislav Sedov schrieb:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On Fri, 01 May 2009 14:02:50 +0200
> Christoph Mallon <christoph.mallon@gmx.de> mentioned:
>
>>> [Don't parenthesize return values]
>>>> Removed, because it does not improve maintainability in any way.
>>> This change could be made and tested mechanically. But there is
>>> no justification for making it - stating that the existing rule
>>> is no better than the proposed rule is no reason to change.
>> Just remove the rule. There's no need to add more redundant parentheses.
>> Again: There is no need to change all existing code at once, but the
>> rule is removed so that not anymore redundant parentheses are added.
>
> If only the rule gets removed this will lead to inconsistent code. Currently
> it is much easier to experienced leader to notice return statements with
> parenthesis around the value than without. Recall that people's eyes are
> build in way that they recognized entire expressions and not letter
> combinations.
I don't buy this for a simple reason: Parentheses are in many statements
(if, while, for). The only thing which distinguishs a return statement
from others is the word "return".
>>>>> +.Sh LOCAL VARIABLES
>>>> Last, but definitely not least, I added this paragraph about the use of
>>>> local variables. This is to clarify, how today's compilers handle
>>>> unaliased local variables.
>>> Every version of gcc that FreeBSD has ever used would do this for
>>> primitive types when optimisation was enabled. This approach can
>>> become expensive in stack (and this is an issue for kernel code) when
>>> using non-primitive types or when optimisation is not enabled (though
>>> I'm not sure if this is supported). Assuming that gcc (and icc and
>>> clang) behaves as stated in all supported optimisation modes, this
>>> change would appear to be quite safe to make.
>> Most local variables are scalars (pointers, ints, ...). A struct or
>> array as local variable is rare and then usually used in one context. So
>> IMO this is fine. Even considereing the extremly rare case of multiple
>> non-scalar declarations in a function, then a compiler can coalesce them
>> if their life ranges are disjoint. It is easier to do so for a compiler
>> to do so, if they are declared with smaller life ranges, example:
>>
>> struct Foo { int x[16]; };
>> struct Bar { int* y[16]; };
>>
>> void f(struct Foo*);
>> void g(struct Bar*);
>>
>> void e(int x)
>> {
>> struct Foo a;
>> struct Bar b;
>> if (x) {
>> f(&a);
>> } else {
>> g(&b);
>> }
>> }
>>
>> Stack usage:
>> subl $140, %esp
>>
>> moving the declarations into the branches:
>> subl $76, %esp
>>
>> So, apart from improved readability, it also can lead to better code.
>> But I consider the latter way less important the benefits observed by a
>> reader of the code.
>>
>
> I particulary like this change.
Why?
> Aliasing behavior is stritcly described in
> ISO C99 standard, so there's a good point to enforcing strict-aliasing clear
> code in our kernel.
If you like this addition because of this reason, I have to disappoint
you: This addition has absolutly *nothing* to do with strict-aliasing.
> On the other hand the big work should be done on clearing
> the existing code before any rule on this can be enforced.
This addition is about improving readability for humans, because it
simplifies the def-use-chains, and as a side effect sometimes leads to
better generated code. It is not sensible to check millions of lines of
code whether a variables are used in different contexts within a
function before this can added.
Anyway, this is moot, because this thread was dead because every
suggested improvement was rejected - especially this last improvement
was rejected by the guy who requested it in the first place.
Christoph
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4A1004B3.5040805>