Re: [PATCH 8/10] Re: [2.6-BK-URL] NTFS: 2.1.19 sparse annotation,cleanups and a bugfix

[Anton Altaparmakov]

On Fri, 24 Sep 2004, Linus Torvalds wrote:
> On Fri, 24 Sep 2004, Anton Altaparmakov wrote:
> > On Fri, 24 Sep 2004, Linus Torvalds wrote:
> > > 
> > > Btw, Al is fixing this. We'll make enum's properly typed, rather than just 
> > > plain integers. It's not traditional C behaviour, but it gives you better 
> > > type safety, and Al points out that other C compilers (the Plan 9 one, to 
> > > be specific) have done the same thing for similar reasons.
> 
> Well, when I said "Al is fixing this", I lied.
> 
> I just fixed it myself. 

Great.  (-:

> > This is good news.  Once that is done I will be very happy to go back to 
> > using enums as I also agree that they can and in this case do look a 
> > lot nicer...
> 
> Try the current sparse, I think it should work for you.
> 
> So if you make an enum where the initializer expression is a little-endian 
> expression, the type of that (single) enumerator will be little-endian.
> 
> HOWEVER, the type of an enum _variable_ will still be just "int". So
> 
> 	enum myenum {
> 		one = 1ULL,
> 		two = 2,
> 	};
> 
> has the strange behaviour that if you use "one" in an expression, it will
> have the type "unsigned long long", but if you use a "enum myenum" entry
> (even if it has the value "1"), it will be an "int":
> 
> 	sizeof(one) == 8
> 	sizeof(enum myenum) == 4
> 
> So I would stronly suggest (and I may make sparse warn) against using
> non-integertyped enum values with any enum that actually has any backing
> store (ie if you ever use a variable of type "enum myenum", that would
> result in a warning - you can really just use the values "one" and "two"
> directly).

Ah, I was using them for backing store as well and I was using the 
__attribute__((packed)) gcc extension to make them the bit-width I wanted 
in combination with a "filler element" at the end of the enum.

So for example to get a 16-bit enum type I was using:

typedef enum {
	RESTART_VOLUME_IS_CLEAN = const_cpu_to_le16(0x0002),
	REST_AREA_SPACE_FILLER  = 0xffff	/* Just to make flags 
16-bit. */
} __attribute__ ((__packed__)) RESTART_AREA_FLAGS;

And then when defining the structure containing these flags I would just 
do:

typedef struct {
	...
	RESTART_AREA_FLAGS flags;
	...
} __attribute__ ((__packed__)) RESTART_AREA;

Also I use the enum type as parameters to functions, for example in the 
above case I might have:
	int blah(RESTART_AREA_FLAGS flags);

So this use doesn't work with the sparse update either.  At the moment I 
have changed everything to just a bunch of #defines followed by a:

typedef le16 RESTART_AREA_FLAGS;

So I guess with your sparse update I can now go to a point in between the 
old one and the new one:

enum {
	RESTART_VOLUME_IS_CLEAN = const_cpu_to_le16(0x0002),
} __attribute__ ((__packed__)) RESTART_AREA_FLAGS;

typedef le16 RESTART_AREA_FLAGS;

So I get the enum rather than bunch of defines and I get my proper types 
as well.

That only looses the ability for the compiler to warn if people use the 
wrong constant when trying to set such a variable or pass a wrong constant 
into a function but that is not nearly as useful a warning as the wrong 
endianness bitwise warnings we have now gained so I am not going to worry 
about losing it.

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/