WARNING: do not add new typedefs - is that for real?

WARNING: do not add new typedefs - is that for real?

[Boaz Harrosh]

I have this code:

<c_code>
/* 
 * osd-r10 4.12.5 Data-In and Data-Out buffer offsets
 * byte offset = mantissa * (2^(exponent+8))
 */
typedef __be32 osd_cdb_offset;

osd_cdb_offset __osd_encode_offset(u64 offset, unsigned *padding,
	int min_shift, int max_shift);

struct osd_attributes_list_mode {
	__be32		get_attr_desc_bytes;
	osd_cdb_offset	get_attr_desc_offset;

	__be32		get_attr_alloc_length;
	osd_cdb_offset	get_attr_offset;

	__be32		set_attr_bytes;
	osd_cdb_offset	set_attr_offset;
	__be32 not_used;
};
</c_code>


the osd_cdb_offset above is this special OSD-standard floating-point-like
special type. It is of size 32 bit in special network order. What should
I do then:

__be32 __osd_encode_offset(u64 offset, unsigned *padding,
	int min_shift, int max_shift);

But it is not a __be32. It is this special floating-point-like thingy!!!?
How was __be32 defined with a #define???!!

Come on guys, it is not checkpatch.pl place to complain about good language
constructs that can be misused. This is the maintainers and reviewers
job to say that a: "typedef struct foo Foo;" is bad practice and we don't
like it, but it can not be left to a script. "typedef"s should be used
where they should be used.

Boaz

Re: WARNING: do not add new typedefs - is that for real?

[Andy Whitcroft]

On Tue, Jan 01, 2008 at 06:15:46PM +0200, Boaz Harrosh wrote:
> I have this code:
> 
> <c_code>
> /* 
>  * osd-r10 4.12.5 Data-In and Data-Out buffer offsets
>  * byte offset = mantissa * (2^(exponent+8))
>  */
> typedef __be32 osd_cdb_offset;
> 
> osd_cdb_offset __osd_encode_offset(u64 offset, unsigned *padding,
> 	int min_shift, int max_shift);
> 
> struct osd_attributes_list_mode {
> 	__be32		get_attr_desc_bytes;
> 	osd_cdb_offset	get_attr_desc_offset;
> 
> 	__be32		get_attr_alloc_length;
> 	osd_cdb_offset	get_attr_offset;
> 
> 	__be32		set_attr_bytes;
> 	osd_cdb_offset	set_attr_offset;
> 	__be32 not_used;
> };
> </c_code>
> 
> 
> the osd_cdb_offset above is this special OSD-standard floating-point-like
> special type. It is of size 32 bit in special network order. What should
> I do then:
> 
> __be32 __osd_encode_offset(u64 offset, unsigned *padding,
> 	int min_shift, int max_shift);
> 
> But it is not a __be32. It is this special floating-point-like thingy!!!?
> How was __be32 defined with a #define???!!
> 
> Come on guys, it is not checkpatch.pl place to complain about good language
> constructs that can be misused. This is the maintainers and reviewers
> job to say that a: "typedef struct foo Foo;" is bad practice and we don't
> like it, but it can not be left to a script. "typedef"s should be used
> where they should be used.

It is checkpatch's role to point out things which are likely to be
wrong.  There will always be exceptions.  Lines whihc are much more
readable if they spill over 80 characters, typedefs which do make sense.
atomic_t's for example.  This may well be a valid use of them.  Note
that this is mentioned as a WARNING not an ERROR.  As is stated in the
patch submission notes, you are meant to be comfortable with everything
which checkpatch is still reporting.

checkpatch is a style _guide_, not the be all and end all.  It is meant
to carry a preferred style to try and maintain some consistency kernel
wide.

-apw

Re: WARNING: do not add new typedefs - is that for real?

[Christoph Hellwig]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=unknown-8bit, Size: 502 bytes --]

On Tue, Jan 01, 2008 at 06:15:46PM +0200, Boaz Harrosh wrote:
> I have this code:
> 
> <c_code>
> /* 
>  * osd-r10 4.12.5 Data-In and Data-Out buffer offsets
>  * byte offset = mantissa * (2^(exponent+8))
>  */
> typedef __be32 osd_cdb_offset;

Given that you can't do normal arithmetic on this type it should't
really be a __be32 but it's own __bitwise type with proper accessors.

But yes, this is one of the rare cases where a typedef makes sense,
but Í'd call it osd_off_t or something like that.

Re: WARNING: do not add new typedefs - is that for real?

[Boaz Harrosh]

On Wed, Jan 02 2008 at 12:08 +0200, Christoph Hellwig <hch@infradead.org> wrote:
> On Tue, Jan 01, 2008 at 06:15:46PM +0200, Boaz Harrosh wrote:
>> I have this code:
>>
>> <c_code>
>> /* 
>>  * osd-r10 4.12.5 Data-In and Data-Out buffer offsets
>>  * byte offset = mantissa * (2^(exponent+8))
>>  */
>> typedef __be32 osd_cdb_offset;
> 
> Given that you can't do normal arithmetic on this type it should't
> really be a __be32 but it's own __bitwise type with proper accessors.
There are all the proper accessors, and arithmetic is certainly not possible.

> 
> But yes, this is one of the rare cases where a typedef makes sense,
> but �'d call it osd_off_t or something like that.
> 

You mean osd_cdb_offset_t. I thought of dropping that _t, I hate it,
just a personal preference.

Point taken about the typedef + __bitwise. Because with __bitwise we
tell the compiler that the new type is assembly equivalent to some type
but otherwise un-mixable and unique type. Checkpatch actually allows
it. Code fixed! Thanks Christoph.

Boaz

Re: WARNING: do not add new typedefs - is that for real?

[Christoph Hellwig]

On Wed, Jan 02, 2008 at 02:18:27PM +0200, Boaz Harrosh wrote:
> > But yes, this is one of the rare cases where a typedef makes sense,
> > but ???'d call it osd_off_t or something like that.
> > 
> 
> You mean osd_cdb_offset_t. I thought of dropping that _t, I hate it,
> just a personal preference.

No, I mean osd_off_t :)  off vs offset is purely cosmetic, but in Linux
we have a strong preference for short typenames.  Just look how ugly
the prototype and struct defintion in your original posting look :)

Also I thing the _cdb is superflous because there is no other offset
type in the OSD spec.