I have function which writes data of a specified type to a buffer. Here is the part of this which writes Uint8
, Uint16
, Uint32
and Uint64
, in big and little endian. As you can see that the same code is repeated several times, so I want to make this code more elegant.
...
case BW_DATA_TYPE_UINT8:
{
Uint8_T val = *(static_cast(Uint8_T*, src));
Uint8ToLittleEndianAr( IN val, OUT_FROM &bw->buffer[bw->curPos], OUT_TO &bw->buffer[bw->curPos] );
bw->curPos += length;
break;
}
case BW_DATA_TYPE_UINT16_LE:
{
Uint16_T val = *(static_cast(Uint16_T*, src));
Uint16ToLittleEndianAr( IN val, OUT_FROM &bw->buffer[bw->curPos], OUT_TO &bw->buffer[bw->curPos] + 1 );
bw->curPos += length;
break;
}
case BW_DATA_TYPE_UINT32_LE:
{
Uint32_T val = *(static_cast(Uint32_T*, src));
Uint32ToLittleEndianAr( IN val, OUT_FROM &bw->buffer[bw->curPos], OUT_TO &bw->buffer[bw->curPos] + 3 );
bw->curPos += length;
break;
}
case BW_DATA_TYPE_UINT64_LE:
{
Uint64_T val = *(static_cast(Uint64_T*, src));
Uint64ToLittleEndianAr( IN val, OUT_FROM &bw->buffer[bw->curPos], OUT_TO &bw->buffer[bw->curPos] + 7 );
bw->curPos += length;
break;
}
case BW_DATA_TYPE_UINT16_BE:
{
Uint16_T val = *(static_cast(Uint16_T*, src));
Uint16ToBigEndianAr( IN val, OUT_FROM &bw->buffer[bw->curPos], OUT_TO &bw->buffer[bw->curPos] + 1 );
bw->curPos += length;
break;
}
case BW_DATA_TYPE_UINT32_BE:
{
Uint32_T val = *(static_cast(Uint32_T*, src));
Uint32ToBigEndianAr( IN val, OUT_FROM &bw->buffer[bw->curPos], OUT_TO &bw->buffer[bw->curPos] + 3 );
bw->curPos += length;
break;
}
case BW_DATA_TYPE_UINT64_BE:
{
Uint64_T val = *(static_cast(Uint64_T*, src));
Uint64ToBigEndianAr( IN val, OUT_FROM &bw->buffer[bw->curPos], OUT_TO &bw->buffer[bw->curPos] + 7 );
bw->curPos += length;
break;
}
...
2 Answers 2
#define CONVERT(T, F, v) T val = *(static_cast(T*, src)); \
F( IN val, OUT_FROM &bw->buffer[bw->curPos], OUT_TO &bw->buffer[bw->curPos] + v ); \
bw->curPos += length;
[...]
case BW_DATA_TYPE_UINT8:
{
CONVERT(Uint8_T, Uint8ToLittleEndianAr, 0)
break;
}
case BW_DATA_TYPE_UINT16_LE:
{
CONVERT(Uint16_T, Uint16ToLittleEndianAr, 1)
break;
}
case BW_DATA_TYPE_UINT32_LE:
{
CONVERT(Uint32_T, Uint32ToLittleEndianAr, 3)
break;
}
case BW_DATA_TYPE_UINT64_LE:
{
CONVERT(Uint64_T, Uint64ToLittleEndianAr, 7)
break;
}
case BW_DATA_TYPE_UINT16_BE:
{
CONVERT(Uint16_T, Uint16ToBigEndianAr, 1)
break;
}
(untested!)
You can go beyond that and even generate the whole case, but this version is a compromise to keep readability.
[EDIT] I missed the bottom of your code, but I suppose you can complete yourself... :-)
-
8\$\begingroup\$ Function-like macros are usually a very bad idea. I think this made the code less readable. \$\endgroup\$Lundin– Lundin2011年09月22日 14:16:18 +00:00Commented Sep 22, 2011 at 14:16
-
\$\begingroup\$ @Lundin, I'm not sure its less readable, but an inline function would be better. \$\endgroup\$Winston Ewert– Winston Ewert2011年09月22日 21:10:56 +00:00Commented Sep 22, 2011 at 21:10
-
\$\begingroup\$ @Winston Ewert: I would go with less readable. Not strictly because it is in the source but because you can't read it from the debugger it is totally opaque from that point of view. Not to mention all the other problems associated with using function macros. \$\endgroup\$Loki Astari– Loki Astari2011年09月23日 00:28:05 +00:00Commented Sep 23, 2011 at 0:28
First of all, I don't think there is anything wrong with the original code. It is clear what it does and the compiler is competent enough to optimize things that are repeated in every case statement.
But if you really must distill this switch into something that only contains the differences between the case statements you can do like this:
// typedef a function pointer, change "type" with the appropriate types for the function.
typedef void(*ToLittleEndianType)(type in, type out_from, type out_to);
Uint8_T val;
Uint8_T offset;
ToLittleEndianType ToLittleEndianAr;
case BW_DATA_TYPE_UINT8:
{
val = *(static_cast(Uint8_T*, src));
offset = 0;
ToLittleEndianAr = Uint8ToLittleEndianAr;
break;
}
case BW_DATA_TYPE_UINT16_LE:
{
val = *(static_cast(Uint16_T*, src));
offset = 1;
ToLittleEndianAr = Uint16ToLittleEndianAr;
break;
}
case BW_DATA_TYPE_UINT32_LE:
{
val = *(static_cast(Uint32_T*, src));
offset = 3;
ToLittleEndianAr = Uint32ToLittleEndianAr;
break;
}
case BW_DATA_TYPE_UINT64_LE:
{
val = *(static_cast(Uint64_T*, src));
offset = 7;
ToLittleEndianAr = Uint64ToLittleEndianAr;
break;
}
case BW_DATA_TYPE_UINT16_BE:
{
val = *(static_cast(Uint16_T*, src));
offset = 1;
ToLittleEndianAr = Uint16ToBigEndianAr;
break;
}
case BW_DATA_TYPE_UINT32_BE:
{
val = *(static_cast(Uint32_T*, src));
offset = 3;
ToLittleEndianAr = Uint32ToBigEndianAr;
break;
}
case BW_DATA_TYPE_UINT64_BE:
{
val = *(static_cast(Uint64_T*, src));
offset = 7;
ToLittleEndianAr = Uint64ToBigEndianAr;
break;
}
ToLittleEndianAr(IN val, OUT_FROM &bw->buffer[bw->curPos], OUT_TO &bw->buffer[bw->curPos] + offset);
bw->curPos += length;
NOTE: This code is type safe, unlike macros.
-
\$\begingroup\$ Will this even work? type in will be different for each function. \$\endgroup\$ronag– ronag2011年09月23日 10:46:13 +00:00Commented Sep 23, 2011 at 10:46
-
\$\begingroup\$ I have no idea what the code is supposed to do, so that's difficult to say. Function pointers can only be used if the functions have the very same parameters. Which in turn means that the functions may or may not need void pointers. It is a C solution, in C++ you could perhaps consider using templates or "functors" instead. \$\endgroup\$Lundin– Lundin2011年09月23日 14:21:14 +00:00Commented Sep 23, 2011 at 14:21
static_cast
s aren't available in C -- but they don't use RTTI. RTTI would only get involved if you useddynamic_cast
. \$\endgroup\$