On Tue, 2017-11-14 at 16:33 -0800, Linus Torvalds wrote: > On Tue, Nov 14, 2017 at 8:36 AM, James Bottomley > <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote: > > > > > > Hannes Reinecke (14): > > scsi: scsi_devinfo: Reformat blacklist flags > > Ugh, that's just really ugly, but it's also wrong. > > Just having long lines would probably have been much preferable, and > would mean that the commit that explains the bit would show up when > you grep for the bit. > > Having a small helper macro like > > #define BLIST_n(x) ((__force __u32 __bitwise)(1 << (n))) > > woiuld also likely have made it more legible. > > But that only takes care of the ugliness and the greppability. > > It's not right for sparse even _with_ those changes. > > Why? Because "__bitwise" actually creates a new type. So what those > BLIST defines should do is to use a special type something like > > typedef unsigned int __bitwise blist_flags_t; > > and now you have _one_ type thanks to that typedef, that is different > from all the other bitwise types. Then you force all the constants > and the field that implements to have that type, and you have type- > safety: you can use those constants together, and you can assign the > result to the blist flags, but you can't mix it with other __bitwise > types. > > That's why things like this work: > > typedef __u16 __bitwise __le16; > typedef __u16 __bitwise __be16; > > where __le16 and __be16 are actually different types, even though > their underlying _storage_ is the same (a 16-bit unsigned). > > Anyway, I've pulled, because clearly this only matters for sparse, > but I would hope that this gets fixed up, ok? It will, boss; I'll make sure of it. James
