2006-05-01 14:00:16

by Jan Engelhardt

[permalink] [raw]
Subject: Re: [PATCH] CodingStyle: add typedefs chapter


>- Chapter 5: Functions
>+ Chapter 5: Typedefs
>+
>+Please don't use things like "vps_t".
>+It's a _mistake_ to use typedef for structures and pointers. When you see a
>+ vps_t a;
>+in the source, what does it mean?
>+In contrast, if it says
>+ struct virtual_container *a;
>+you can actually tell what "a" is.
>+
>+Lots of people think that typedefs "help readability". Not so. They are
>+useful only for:
[...]

What about task_t vs struct task_struct? Both are used in the kernel.



Jan Engelhardt
--


2006-05-01 14:21:00

by Alexey Dobriyan

[permalink] [raw]
Subject: Re: [PATCH] CodingStyle: add typedefs chapter

On Mon, May 01, 2006 at 04:00:09PM +0200, Jan Engelhardt wrote:
> >+Please don't use things like "vps_t".
> >+It's a _mistake_ to use typedef for structures and pointers. When you see a
> >+ vps_t a;
> >+in the source, what does it mean?
> >+In contrast, if it says
> >+ struct virtual_container *a;
> >+you can actually tell what "a" is.
> >+
> >+Lots of people think that typedefs "help readability". Not so. They are
> >+useful only for:
> [...]
>
> What about task_t vs struct task_struct? Both are used in the kernel.

task_t => struct task
struct task_struct => struct task

Roughly 2765 hits :-\

2006-05-01 14:46:50

by linux-os (Dick Johnson)

[permalink] [raw]
Subject: Re: [PATCH] CodingStyle: add typedefs chapter


On Mon, 1 May 2006, Alexey Dobriyan wrote:

> On Mon, May 01, 2006 at 04:00:09PM +0200, Jan Engelhardt wrote:
>>> +Please don't use things like "vps_t".
>>> +It's a _mistake_ to use typedef for structures and pointers. When you see a
>>> + vps_t a;
>>> +in the source, what does it mean?
>>> +In contrast, if it says
>>> + struct virtual_container *a;
>>> +you can actually tell what "a" is.
>>> +
>>> +Lots of people think that typedefs "help readability". Not so. They are
>>> +useful only for:
>> [...]
>>
>> What about task_t vs struct task_struct? Both are used in the kernel.
>
> task_t => struct task
> struct task_struct => struct task
>
> Roughly 2765 hits :-\

Yes, also 'current' is probably the most used. Any, since this
has become a FAQ, maybe it's about time to put something in the
Documentation?

--- /usr/src/linux-2.6.16.4/Documentation/CodingStyle.orig 2006-05-01 10:17:03.000000000 -0400
+++ /usr/src/linux-2.6.16.4/Documentation/CodingStyle 2006-05-01 10:37:09.000000000 -0400
@@ -343,6 +343,33 @@
Remember: if another thread can find your data structure, and you don't
have a reference count on it, you almost certainly have a bug.

+ typedefs and and structs
+
+Typedefs should never be used for information hiding. In other words,
+if a typedef defines an aggregate type, and the individual components
+are accessed anywhere in the code, a typedef should not be used.
+
+An example of proper usage:
+typedef struct opaque_type FILE; // In a header
+
+ FILE *fp; // In a program block
+
+The type 'FILE' in this example is something that was defined in
+a 'C' runtime library. The code uses pointers to this opaque type,
+but never even knows, and doesn't care, what's inside that structure.
+Therefore, FILE can have been defined using a typedef.
+
+An example of incorrect usage:
+
+typedef struct file_operations FO; // In a header
+
+ FO fo; // In a program block
+ memset(&foo, 0x00, sizeof(foo));
+
+In this case, object FO contains structure members that will be
+accessed by the code. It should not have been defined. Instead,
+the structure name should have been used directly.
+

Chapter 11: Macros, Enums and RTL



In case the M$ server mangles the patch, it's included as an attachment.


Cheers,
Dick Johnson
Penguin : Linux version 2.6.16.4 on an i686 machine (5592.89 BogoMips).
New book: http://www.lymanschool.com
_



****************************************************************
The information transmitted in this message is confidential and may be privileged. Any review, retransmission, dissemination, or other use of this information by persons or entities other than the intended recipient is prohibited. If you are not the intended recipient, please notify Analogic Corporation immediately - by replying to this message or by sending an email to [email protected] - and destroy all copies of this information, including any attachments, without reading or disclosing them.

Thank you.


Attachments:
style.patch (1.35 kB)
style.patch

2006-05-01 15:00:55

by Jiri Slaby

[permalink] [raw]
Subject: Re: [PATCH] CodingStyle: add typedefs chapter

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

linux-os (Dick Johnson) napsal(a):
> On Mon, 1 May 2006, Alexey Dobriyan wrote:
>
>> On Mon, May 01, 2006 at 04:00:09PM +0200, Jan Engelhardt wrote:
>>>> +Please don't use things like "vps_t".
>>>> +It's a _mistake_ to use typedef for structures and pointers. When you see a
>>>> + vps_t a;
>>>> +in the source, what does it mean?
>>>> +In contrast, if it says
>>>> + struct virtual_container *a;
>>>> +you can actually tell what "a" is.
>>>> +
>>>> +Lots of people think that typedefs "help readability". Not so. They are
>>>> +useful only for:
>>> [...]
>>>
>>> What about task_t vs struct task_struct? Both are used in the kernel.
>> task_t => struct task
>> struct task_struct => struct task
>>
>> Roughly 2765 hits :-\
>
> Yes, also 'current' is probably the most used. Any, since this
> has become a FAQ, maybe it's about time to put something in the
> Documentation?
>
> --- /usr/src/linux-2.6.16.4/Documentation/CodingStyle.orig 2006-05-01 10:17:03.000000000 -0400
> +++ /usr/src/linux-2.6.16.4/Documentation/CodingStyle 2006-05-01 10:37:09.000000000 -0400
> @@ -343,6 +343,33 @@
> Remember: if another thread can find your data structure, and you don't
> have a reference count on it, you almost certainly have a bug.
>
> + typedefs and and structs
> +
> +Typedefs should never be used for information hiding. In other words,
> +if a typedef defines an aggregate type, and the individual components
> +are accessed anywhere in the code, a typedef should not be used.
> +
> +An example of proper usage:
> +typedef struct opaque_type FILE; // In a header
> +
> + FILE *fp; // In a program block
> +
> +The type 'FILE' in this example is something that was defined in
> +a 'C' runtime library. The code uses pointers to this opaque type,
> +but never even knows, and doesn't care, what's inside that structure.
> +Therefore, FILE can have been defined using a typedef.
> +
> +An example of incorrect usage:
> +
> +typedef struct file_operations FO; // In a header
> +
> + FO fo; // In a program block
> + memset(&foo, 0x00, sizeof(foo));
> +
> +In this case, object FO contains structure members that will be
> +accessed by the code. It should not have been defined. Instead,
> +the structure name should have been used directly.
> +
>
> Chapter 11: Macros, Enums and RTL
>
>
>
> In case the M$ server mangles the patch, it's included as an attachment.
>
>
> Cheers,
> Dick Johnson
> Penguin : Linux version 2.6.16.4 on an i686 machine (5592.89 BogoMips).
> New book: http://www.lymanschool.com
> _
>
>
>
> ****************************************************************
> The information transmitted in this message is confidential and may be privileged. Any review, retransmission, dissemination, or other use of this information by persons or entities other than the intended recipient is prohibited. If you are not the intended recipient, please notify Analogic Corporation immediately - by replying to this message or by sending an email to [email protected] - and destroy all copies of this information, including any attachments, without reading or disclosing them.
>
> Thank you.
>
>
> ------------------------------------------------------------------------
>
> --- /usr/src/linux-2.6.16.4/Documentation/CodingStyle.orig 2006-05-01 10:17:03.000000000 -0400
> +++ /usr/src/linux-2.6.16.4/Documentation/CodingStyle 2006-05-01 10:37:09.000000000 -0400
> @@ -343,6 +343,33 @@
> Remember: if another thread can find your data structure, and you don't
> have a reference count on it, you almost certainly have a bug.
>
> + typedefs and and structs
> +
> +Typedefs should never be used for information hiding. In other words,
> +if a typedef defines an aggregate type, and the individual components
> +are accessed anywhere in the code, a typedef should not be used.
> +
> +An example of proper usage:
> +typedef struct opaque_type FILE; // In a header
> +
> + FILE *fp; // In a program block
> +
> +The type 'FILE' in this example is something that was defined in
> +a 'C' runtime library. The code uses pointers to this opaque type,
> +but never even knows, and doesn't care, what's inside that structure.
> +Therefore, FILE can have been defined using a typedef.
> +
> +An example of incorrect usage:
> +
> +typedef struct file_operations FO; // In a header
> +
> + FO fo; // In a program block
You meant 'FO foo', didn't you?
> + memset(&foo, 0x00, sizeof(foo));
> +
> +In this case, object FO contains structure members that will be
> +accessed by the code. It should not have been defined. Instead,
> +the structure name should have been used directly.
> +
>
> Chapter 11: Macros, Enums and RTL
>

regards,
- --
Jiri Slaby http://www.fi.muni.cz/~xslaby
\_.-^-._ [email protected] _.-^-._/
B67499670407CE62ACC8 22A032CC55C339D47A7E
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.3 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org

iD8DBQFEViK1MsxVwznUen4RAi8wAJ9km1HybAI8qRUR5IY9y52+LDHdogCfSy4O
ENhfqjTKF/+zdyjmuV7wXws=
=yZTA
-----END PGP SIGNATURE-----

2006-05-01 15:33:44

by Artem B. Bityutskiy

[permalink] [raw]
Subject: Re: [PATCH] CodingStyle: add typedefs chapter

Jan Engelhardt wrote:
> What about task_t vs struct task_struct? Both are used in the kernel.

If you use task_struct's fields, pick struct task_struct, otherwise pick
task_t.

--
Best regards, Artem B. Bityutskiy
Oktet Labs (St. Petersburg), Software Engineer.
+7 812 4286709 (office) +7 911 2449030 (mobile)
E-mail: [email protected], Web: http://www.oktetlabs.ru

2006-05-01 16:58:49

by David Woodhouse

[permalink] [raw]
Subject: Re: [PATCH] CodingStyle: add typedefs chapter

On Mon, 2006-05-01 at 16:00 +0200, Jan Engelhardt wrote:
> What about task_t vs struct task_struct? Both are used in the kernel.

task_t should probably die.

--
dwmw2

2006-05-01 20:20:22

by Jan Engelhardt

[permalink] [raw]
Subject: Re: [PATCH] CodingStyle: add typedefs chapter


>> What about task_t vs struct task_struct? Both are used in the kernel.
>
>task_t should probably die.

Acked.



Jan Engelhardt
--

2006-05-01 20:44:15

by Jiri Slaby

[permalink] [raw]
Subject: Re: [PATCH] CodingStyle: add typedefs chapter

Jan Engelhardt napsal(a):
>>> What about task_t vs struct task_struct? Both are used in the kernel.
>> task_t should probably die.
>
> Acked.
Wouldn't this be a janitors' task?

regards,
--
Jiri Slaby http://www.fi.muni.cz/~xslaby
\_.-^-._ [email protected] _.-^-._/
B67499670407CE62ACC8 22A032CC55C339D47A7E

2006-05-01 21:01:54

by Jan Engelhardt

[permalink] [raw]
Subject: Re: [PATCH] CodingStyle: add typedefs chapter

>>>> What about task_t vs struct task_struct? Both are used in the kernel.
>>> task_t should probably die.
>>
>> Acked.
>Wouldn't this be a janitors' task?

Too simple :)

find rc3 -type f -print0 | xargs -0 perl -i -pe
's/\btask_t\b/struct task_struct'

+ a compile test afterwards. Something I missed? (Besides that lines may
get longer and violate the 80-column CodingStyle rule.)


Jan Engelhardt
--

2006-05-01 21:06:51

by David Woodhouse

[permalink] [raw]
Subject: Re: [PATCH] CodingStyle: add typedefs chapter

On Mon, 2006-05-01 at 23:01 +0200, Jan Engelhardt wrote:
> find rc3 -type f -print0 | xargs -0 perl -i -pe
> 's/\btask_t\b/struct task_struct'
>
> + a compile test afterwards. Something I missed? (Besides that lines
> may get longer and violate the 80-column CodingStyle rule.)

If we're going to do that, we might as well make it 'struct task'. The
additional '_struct' is redundant.

--
dwmw2

2006-05-01 21:40:58

by Alexey Dobriyan

[permalink] [raw]
Subject: Re: [PATCH] CodingStyle: add typedefs chapter

On Mon, May 01, 2006 at 10:07:29PM +0100, David Woodhouse wrote:
> On Mon, 2006-05-01 at 23:01 +0200, Jan Engelhardt wrote:
> > find rc3 -type f -print0 | xargs -0 perl -i -pe
> > 's/\btask_t\b/struct task_struct'
> >
> > + a compile test afterwards. Something I missed? (Besides that lines
> > may get longer and violate the 80-column CodingStyle rule.)
>
> If we're going to do that, we might as well make it 'struct task'. The
> additional '_struct' is redundant.

struct sighand_struct
signal_struct
files_struct
fs_struct
sel_arg_struct
mmap_arg_struct
vm_area_struct
tty_struct
fasync_struct
rose_facilities_struct
poll_table_struct
...

What is the best day for Grand Renaming?

2006-05-02 10:41:04

by Jörn Engel

[permalink] [raw]
Subject: Re: [PATCH] CodingStyle: add typedefs chapter

On Tue, 2 May 2006 01:38:59 +0400, Alexey Dobriyan wrote:
>
> What is the best day for Grand Renaming?

Usually when you're in the area changing something anyway.

J?rn

--
Good warriors cause others to come to them and do not go to others.
-- Sun Tzu

2006-05-02 13:17:38

by Jes Sorensen

[permalink] [raw]
Subject: Re: [PATCH] CodingStyle: add typedefs chapter

>>>>> "David" == David Woodhouse <[email protected]> writes:

David> On Mon, 2006-05-01 at 23:01 +0200, Jan Engelhardt wrote:
>> find rc3 -type f -print0 | xargs -0 perl -i -pe
>> 's/\btask_t\b/struct task_struct'
>>
>> + a compile test afterwards. Something I missed? (Besides that
>> lines may get longer and violate the 80-column CodingStyle rule.)

David> If we're going to do that, we might as well make it 'struct
David> task'. The additional '_struct' is redundant.

There were some long discussions about that a couple of years ago. I
believe Linus stated that he preferred _struct for those things.

It is also less likely to hit name clashes.

Jes