Style Question

Style Question

[Cong WANG]

Hi, list!

I have a question about coding style in linux kernel. In
Documention/CodingStyle, it is said that "Linux style for comments is
the C89 "/* ... */" style. Don't use C99-style "// ..." comments."
_But_ I see a lot of '//' style comments in current kernel code.

Which is wrong? The documentions or the code, or neither? And why?

Another question is about NULL. AFAIK, in user space, using NULL is
better than directly using 0 in C. In kernel, I know it used its own
NULL, which may be defined as ((void*)0), but it's _still_ different
from raw zero. So can I say using NULL is better than 0 in kernel?

Any reply is welcome. Thanks and have a nice day!

Re: Style Question

[Bernd Petrovitsch]

On Sun, 2007-03-11 at 22:15 +0800, Cong WANG wrote:
[...]
> Another question is about NULL. AFAIK, in user space, using NULL is
> better than directly using 0 in C. In kernel, I know it used its own
> NULL, which may be defined as ((void*)0),

Userspace has the usually same definition.

>                                           but it's _still_ different
> from raw zero.

It is different that "0" as such has the type "int". But this int is
automatically promoted to a "0 pointer".

>                So can I say using NULL is better than 0 in kernel?

Yes, because it is immediately clear that a pointer is (or should be)
there (and not an int).
And the same holds for userspace since this is a pure C question.

	Bernd
-- 
Firmix Software GmbH                   http://www.firmix.at/
mobil: +43 664 4416156                 fax: +43 1 7890849-55
          Embedded Linux Development and Services

Re: Style Question

[Jan Engelhardt]

On Mar 11 2007 22:15, Cong WANG wrote:
>
> I have a question about coding style in linux kernel. In
> Documention/CodingStyle, it is said that "Linux style for comments is
> the C89 "/* ... */" style. Don't use C99-style "// ..." comments."
> _But_ I see a lot of '//' style comments in current kernel code.
>
> Which is wrong? The documentions or the code, or neither? And why?

The code. And because it's not always reviewed but silently pushed.

> Another question is about NULL. AFAIK, in user space, using NULL is
> better than directly using 0 in C. In kernel, I know it used its own
> NULL, which may be defined as ((void*)0), but it's _still_ different
> from raw zero.

In what way?

>So can I say using NULL is better than 0 in kernel?

On what basis? Do you even know what NULL is defined as in
(C, not C++) userspace? Think about it.


Jan
-- 

Re: Style Question

[Daniel Hazelton]

On Sunday 11 March 2007 16:35:50 Jan Engelhardt wrote:
> On Mar 11 2007 22:15, Cong WANG wrote:
> > Another question is about NULL. AFAIK, in user space, using NULL is
> > better than directly using 0 in C. In kernel, I know it used its own
> > NULL, which may be defined as ((void*)0), but it's _still_ different
> > from raw zero.
>
> In what way?
>
> >So can I say using NULL is better than 0 in kernel?
>
> On what basis? Do you even know what NULL is defined as in
> (C, not C++) userspace? Think about it.

IIRC, the glibc and GCC headers define NULL as (void*)0  :)

>
>
> Jan

Re: Style Question

[Kyle Moffett]

On Mar 11, 2007, at 16:41:51, Daniel Hazelton wrote:
> On Sunday 11 March 2007 16:35:50 Jan Engelhardt wrote:
>> On Mar 11 2007 22:15, Cong WANG wrote:
>>> So can I say using NULL is better than 0 in kernel?
>>
>> On what basis? Do you even know what NULL is defined as in (C, not  
>> C++) userspace? Think about it.
>
> IIRC, the glibc and GCC headers define NULL as (void*)0  :)

On the other hand when __cplusplus is defined they define it to the  
"__null" builtin, which GCC uses to give type conversion errors for  
"int foo = NULL" but not "char *foo = NULL".  A "((void *)0)"  
definition gives C++ type errors for both due to the broken C++ void  
pointer conversion problems.

Cheers,
Kyle Moffett

Re: Style Question

[Jan Engelhardt]

On Mar 11 2007 18:01, Kyle Moffett wrote:
> On Mar 11, 2007, at 16:41:51, Daniel Hazelton wrote:
>> On Sunday 11 March 2007 16:35:50 Jan Engelhardt wrote:
>> > On Mar 11 2007 22:15, Cong WANG wrote:
>> > > So can I say using NULL is better than 0 in kernel?
>> > 
>> > On what basis? Do you even know what NULL is defined as in (C, not
>> > C++) userspace? Think about it.
>> 
>> IIRC, the glibc and GCC headers define NULL as (void*)0  :)
>
> On the other hand when __cplusplus is defined they define it to the
> "__null" builtin, which GCC uses to give type conversion errors for
> "int foo = NULL" but not "char *foo = NULL".  A "((void *)0)"
> definition gives C++ type errors for both due to the broken C++
> void pointer conversion problems.

I think that the primary reason they use __null is so that you can
actually do

	class foo *ptr = NULL;

because

	class foo *ptr = (void *)0;

would throw an error or at least a warning (implicit cast from void*
to class foo*).


Jan
-- 

Re: Style Question

[Kyle Moffett]

On Mar 11, 2007, at 19:16:59, Jan Engelhardt wrote:
> On Mar 11 2007 18:01, Kyle Moffett wrote:
>> On the other hand when __cplusplus is defined they define it to  
>> the "__null" builtin, which GCC uses to give type conversion  
>> errors for "int foo = NULL" but not "char *foo = NULL".  A "((void  
>> *)0)" definition gives C++ type errors for both due to the broken C 
>> ++ void pointer conversion problems.
>
> I think that the primary reason they use __null is so that you can
> actually do
>
> 	class foo *ptr = NULL;
>
> because
>
> 	class foo *ptr = (void *)0;
>
> would throw an error or at least a warning (implicit cast from void*
> to class foo*).

Isn't that what I said? :-D

Cheers,
Kyle Moffett

Re: Style Question

[Jan Engelhardt]

On Mar 11 2007 21:27, Kyle Moffett wrote:
> On Mar 11, 2007, at 19:16:59, Jan Engelhardt wrote:
>> On Mar 11 2007 18:01, Kyle Moffett wrote:
>> > On the other hand when __cplusplus is defined they define it to the
>> > "__null" builtin, which GCC uses to give type conversion errors for
>> > "int foo = NULL" but not "char *foo = NULL".

>> I think that the primary reason they use __null is so that you can
>> actually do[...]
>
> Isn't that what I said? :-D

Ya. Though I was picking at

|"__null" builtin, which GCC uses to give type conversion errors for
|"int foo = NULL"

since C's (void *)0 would also barf when being assigned to int.
So it's not a genuine __null feature ;-)


Jan
-- 

Re: Style Question

[Kyle Moffett]

On Mar 11, 2007, at 21:32:00, Jan Engelhardt wrote:
> On Mar 11 2007 21:27, Kyle Moffett wrote:
>> On Mar 11, 2007, at 19:16:59, Jan Engelhardt wrote:
>>> On Mar 11 2007 18:01, Kyle Moffett wrote:
>>>> On the other hand when __cplusplus is defined they define it to the
>>>> "__null" builtin, which GCC uses to give type conversion errors for
>>>> "int foo = NULL" but not "char *foo = NULL".
>
>>> I think that the primary reason they use __null is so that you can
>>> actually do[...]
>>
>> Isn't that what I said? :-D
>
> Ya. Though I was picking at
>
>> "__null" builtin, which GCC uses to give type conversion errors  
>> for "int foo = NULL"
>
> since C's (void *)0 would also barf when being assigned to int.  So  
> it's not a genuine __null feature ;-)

You chopped my sentence in half! :-D  What I *really* said was:
> ...give type conversion errors for 'int foo = NULL' but not 'char  
> *foo = NULL'.

The pseudo-standard "#define NULL (0)" that the C++ standards ask for  
does *NOT* give an error for "int foo = NULL;", and in C++ the C- 
standard "#define NULL ((void *)0)" *does* give an error for "char  
*foo = NULL;"  Ergo I think I was correct when I said "GCC uses  
[__null] to give type conversion errors for <the-first> but not <the- 
second>"

Cheers,
Kyle Moffett

Re: Style Question

[Cong WANG]

2007/3/12, Jan Engelhardt <jengelh@linux01.gwdg.de>:
>
> On Mar 11 2007 22:15, Cong WANG wrote:
> >
> > I have a question about coding style in linux kernel. In
> > Documention/CodingStyle, it is said that "Linux style for comments is
> > the C89 "/* ... */" style. Don't use C99-style "// ..." comments."
> > _But_ I see a lot of '//' style comments in current kernel code.
> >
> > Which is wrong? The documentions or the code, or neither? And why?
>
> The code. And because it's not always reviewed but silently pushed.
>
> > Another question is about NULL. AFAIK, in user space, using NULL is
> > better than directly using 0 in C. In kernel, I know it used its own
> > NULL, which may be defined as ((void*)0), but it's _still_ different
> > from raw zero.
>
> In what way?

The following code is picked from drivers/kvm/kvm_main.c:

static struct kvm_vcpu *vcpu_load(struct kvm *kvm, int vcpu_slot)
{
       struct kvm_vcpu *vcpu = &kvm->vcpus[vcpu_slot];

       mutex_lock(&vcpu->mutex);
       if (unlikely(!vcpu->vmcs)) {
               mutex_unlock(&vcpu->mutex);
               return 0;
       }
       return kvm_arch_ops->vcpu_load(vcpu);
}

Obviously, it used 0 rather than NULL when returning a pointer to
indicate an error. Should we fix such issue?

>
> >So can I say using NULL is better than 0 in kernel?
>
> On what basis? Do you even know what NULL is defined as in
> (C, not C++) userspace? Think about it.
>

I think it's more clear to indicate we are using a pointer rather than
an integer when we use NULL in kernel. But in userspace, using NULL is
for portbility of the program, although most (*just* most, NOT all) of
NULL's defination is ((void*)0). ;-)

Re: Style Question

[Jan Engelhardt]

On Mar 12 2007 13:37, Cong WANG wrote:
>
> The following code is picked from drivers/kvm/kvm_main.c:
>
> static struct kvm_vcpu *vcpu_load(struct kvm *kvm, int vcpu_slot)
> {
> struct kvm_vcpu *vcpu = &kvm->vcpus[vcpu_slot];
>
> mutex_lock(&vcpu->mutex);
> if (unlikely(!vcpu->vmcs)) {
> mutex_unlock(&vcpu->mutex);
> return 0;
> }
> return kvm_arch_ops->vcpu_load(vcpu);
> }
>
> Obviously, it used 0 rather than NULL when returning a pointer to
> indicate an error. Should we fix such issue?

Indeed. If it was for me, something like that should throw a compile error.

>>[...]
> I think it's more clear to indicate we are using a pointer rather than
> an integer when we use NULL in kernel. But in userspace, using NULL is
> for portbility of the program, although most (*just* most, NOT all) of
> NULL's defination is ((void*)0). ;-)

NULL has the same bit pattern as the number zero. (I'm not saying the bit
pattern is all zeroes. And I am not even sure if NULL ought to have the same
pattern as zero.) So C++ could use (void *)0, if it would let itself :p


>
>

Jan
-- 

Re: Style Question

[Nicholas Miell]

On Mon, 2007-03-12 at 06:40 +0100, Jan Engelhardt wrote:
> On Mar 12 2007 13:37, Cong WANG wrote:
> >
> > The following code is picked from drivers/kvm/kvm_main.c:
> >
> > static struct kvm_vcpu *vcpu_load(struct kvm *kvm, int vcpu_slot)
> > {
> > struct kvm_vcpu *vcpu = &kvm->vcpus[vcpu_slot];
> >
> > mutex_lock(&vcpu->mutex);
> > if (unlikely(!vcpu->vmcs)) {
> > mutex_unlock(&vcpu->mutex);
> > return 0;
> > }
> > return kvm_arch_ops->vcpu_load(vcpu);
> > }
> >
> > Obviously, it used 0 rather than NULL when returning a pointer to
> > indicate an error. Should we fix such issue?
> 
> Indeed. If it was for me, something like that should throw a compile error.
> 
> >>[...]
> > I think it's more clear to indicate we are using a pointer rather than
> > an integer when we use NULL in kernel. But in userspace, using NULL is
> > for portbility of the program, although most (*just* most, NOT all) of
> > NULL's defination is ((void*)0). ;-)
> 
> NULL has the same bit pattern as the number zero. (I'm not saying the bit
> pattern is all zeroes. And I am not even sure if NULL ought to have the same
> pattern as zero.) So C++ could use (void *)0, if it would let itself :p

Not necessarily. You can use 0 at the source level, but the compiler has
to convert it to the actual NULL pointer bit pattern, whatever it may
be.

In C++, NULL is typically defined to 0 (with no void* cast) by most
compilers because 0 (and only 0) can be implicitly converted to to null
pointer of any ponter type without a cast. 

GCC introduced the __null extension so that NULL still works correctly
in C++ when passed to a varargs function on 64-bit platforms.

(This just works in C because C makes NULL ((void*)0) is thus is the
right size. In C++, the 0 ends up being an int instead of a pointer when
passed to a varargs function, and things tend to blow up when they read
the garbage high bits. Of course, nobody else does this, so you still
have to use (void*)NULL to be portable.)

-- 
Nicholas Miell <nmiell@comcast.net>

Re: Style Question

[Randy.Dunlap]

On Mon, 12 Mar 2007, Jan Engelhardt wrote:

>
> On Mar 12 2007 13:37, Cong WANG wrote:
> >
> > The following code is picked from drivers/kvm/kvm_main.c:
> >
> > static struct kvm_vcpu *vcpu_load(struct kvm *kvm, int vcpu_slot)
> > {
> > struct kvm_vcpu *vcpu = &kvm->vcpus[vcpu_slot];
> >
> > mutex_lock(&vcpu->mutex);
> > if (unlikely(!vcpu->vmcs)) {
> > mutex_unlock(&vcpu->mutex);
> > return 0;
> > }
> > return kvm_arch_ops->vcpu_load(vcpu);
> > }
> >
> > Obviously, it used 0 rather than NULL when returning a pointer to
> > indicate an error. Should we fix such issue?
>
> Indeed. If it was for me, something like that should throw a compile error.

At least it does throw a sparse warning, and yes, it should
be fixed.

> >>[...]
> > I think it's more clear to indicate we are using a pointer rather than
> > an integer when we use NULL in kernel. But in userspace, using NULL is
> > for portbility of the program, although most (*just* most, NOT all) of
> > NULL's defination is ((void*)0). ;-)
>
> NULL has the same bit pattern as the number zero. (I'm not saying the bit
> pattern is all zeroes. And I am not even sure if NULL ought to have the same
> pattern as zero.) So C++ could use (void *)0, if it would let itself :p
>
>
> >
> >
>
> Jan
>

-- 
~Randy

Re: Style Question

[Cong WANG]

2007/3/12, David Schwartz <davids@webmaster.com>:
>
> > NULL has the same bit pattern as the number zero. (I'm not saying the bit
> > pattern is all zeroes. And I am not even sure if NULL ought to
> > have the same
> > pattern as zero.) So C++ could use (void *)0, if it would let itself :p
>
> They don't have to have the same bit pattern. There's no logical reason a
> NULL pointer couldn't have all bits set and the number zero have all bits
> cleared.
>
> Casts are perrmited to change the bit pattern. For example '(float) 7' can
> result in a different bit pattern than '7' and similarly '(void *) 0' can
> result in a different bit pattern from '0'.
>
> As a trivial example, consider an LP64 system. NULL will have the bit
> pattern of 64 zero bits, while '0' will have the bit pattern of 32 zero
> bits.
>
> DS

I agree. C99 standard just says:

6.3.2.3 Pointers

"3  An integer constant expression with the value 0, or such an
expression cast to type void *, is called a null pointer constant. If
a null pointer constant is converted to a pointer type, the resulting
pointer, called a null pointer, is guaranteed to compare unequal to a
pointer to any object or function."

Re: Style Question

[Robert Hancock]

Cong WANG wrote:
> Hi, list!
> 
> I have a question about coding style in linux kernel. In
> Documention/CodingStyle, it is said that "Linux style for comments is
> the C89 "/* ... */" style. Don't use C99-style "// ..." comments."
> _But_ I see a lot of '//' style comments in current kernel code.
> 
> Which is wrong? The documentions or the code, or neither? And why?

The code.. As with a lot of coding style issues, it's likely just that 
nobody saw it and bothered to complain when it went in.

> Another question is about NULL. AFAIK, in user space, using NULL is
> better than directly using 0 in C. In kernel, I know it used its own
> NULL, which may be defined as ((void*)0), but it's _still_ different
> from raw zero. So can I say using NULL is better than 0 in kernel?

It's the preferred style, Sparse will complain about using 0 for a null 
pointer for example..

-- 
Robert Hancock      Saskatoon, SK, Canada
To email, remove "nospam" from hancockr@nospamshaw.ca
Home Page: http://www.roberthancock.com/