Messages in this thread | | | Date | Thu, 16 Sep 2004 07:01:20 -0700 (PDT) | From | Linus Torvalds <> | Subject | Re: Being more careful about iospace accesses.. |
| |
On Thu, 16 Sep 2004, David Woodhouse wrote:
> On Wed, 2004-09-15 at 16:26 -0700, Linus Torvalds wrote: > > - if you want to go outside that bitwise type, you have to convert it > > properly first. For example, if you want to add a constant to a __le16 > > type, you can do so, but you have to use the proper sequence: > > > > __le16 sum, a, b; > > > > sum = a + b; /* INVALID! "warning: incompatible types for operation (+)" */ > > sum = cpu_to_le16(le16_to_cpu(a) + le16_to_cpu(b)); /* Ok */ > > > > See? > > Yeah right, that latter case is _so_ much more readable
It's not about readability.
It's about the first case being WRONG!
You can't add two values in the wrong byte-order. It's not an operation that makes sense. You _have_ to convert them to CPU byte order first.
I certainly agree that the first version "looks nicer".
> It's even nicer when it ends up as: > > sum = cpu_to_le16(le16_to_cpu(a) + le16_to_cpu(b)); /* Ok */ > sum |= c; > sum = cpu_to_le16(le16_to_cpu(sum) + le16_to_cpu(d));
This is actually the strongest argument _against_ hiding endianness in the compiler, or hiding it behind macros. Make it very explicit, and just make sure there are tools (ie 'sparse') that can tell you when you do something wrong.
Any programmer who sees the above will go "well that's stupid", and rewrite it as something saner instead. You can certainly rewrite it as
cpu_sum = le16_to_cpu(a) + le16_to_cpu(b); cpu_sum |= le16_to_cpu(c); cpu_sum += le16_to_cpu(d); sum = cpu_to_le16(d);
which gets rid of the double conversions.
But if you hide the endianness in macro's, you'll never see the mess at all, and won't be able to fix it.
> I'd really quite like to see the real compiler know about endianness, > too.
I would have agreed with you some time ago. Having been bitten by too damn many bompiler bugs I'e become convinced that the compiler doing things behind your back to "help" you just isn't worth it. Not in a kernel, at least. It's much better to build up good typechecking and the infrastructure to help you get the job done.
Expressions like the above might happen once or twice in a project with several million lines of code. It's just not worth compiler infrastructure for - that just makes people use it as if it is free, and impossible to find the bugs when they _do_ happen. Much better to have a type system that can warn about the bad uses, but that doesn't actually change any of the code generated..
Linus - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |