[pisa-src] Re: r1640 - trunk/libpisa/checksum.h
- From: Tobias Heer <heer@xxxxxxxxxxxxxxxxx>
- To: pisa-src@xxxxxxxxxxxxx
- Date: Wed, 11 Nov 2009 15:42:31 +0100
Am 11.11.2009 um 14:59 schrieb Thomas Jansen:
On Wed, Nov 11, 2009 at 02:21:56PM +0100, Diego Biurrun wrote:
+/* TODO: needs to include a header for uint{16,32}_t to be
selfcontained */
The header that contains the typedefs for uint16_t?
That header would be stdint.h.
Fixed in 1649, thanks.
typedef uint16_t pisa_csum16;
typedef uint32_t pisa_csum32;
I think such typedefs are harmful obfuscation. Now if I read a
function
that uses pisa_csum32 I have to read some context to understand it
completely. Why would we need such an extra layer of indirection?
I think it's useful to distinguish between a generic type (uint...)
and the
type representing a certain use case for the generic type
(pisa_csum...).
Especially when we have a function like
void pisa_csum_replace4(pisa_csum16 *addr, uint32_t before, uint32_t
after)
it's easy to see that before and after are not checksums but actual
data.
Wouldn't it make much more sense to use descriptive parameter names
instead? It's true that it is really hard to see what happens in:
pisa_csum_replace4(pisa_csum16 *addr, uint32_t before, uint32_t after)
... but that's not because of the tyoe of addr but because of the
name. Why not call it checksum instead???
@@ -21,7 +23,7 @@
-static inline uint16_t pisa_csum_fold(pisa_csum32 c)
+static inline pisa_csum16 pisa_csum_fold(pisa_csum32 c)
As I said, I think this was more readable before.
In that case you should complain about using pisa_csum32 as the type
for the
argument. ;) I changed this to match the rest of the file, where the
types
are used "properly". Unless we drop the typedefs altogether, we
should be
consistent in their use.
--
Thomas Jansen, "Mithi" --- mithi@xxxxxxxxx
GPG 9D5C682B, feel free to sign or encrypt your mail
--
Dipl.-Inform. Tobias Heer, Ph.D. Student
Distributed Systems Group
RWTH Aachen University, Germany
tel: +49 241 80 207 76
web: http://ds.cs.rwth-aachen.de/members/heer
Other related posts:
