I have encountered a problem, and I haven't found an answer in the internet and forums. I hope you can help.
There is an interface requirement to update the LOG parameter. The interface passes in the parameter index and the value to be added, and the interface adds the value to the corresponding parameter. The interface requires simple implementation and no complicated judgments.
My idea is to create a mapping table that records the starting address and data type of each parameter. When the interface is called, the parameter address is obtained according to the parameter index and forced to be converted to the corresponding type, and then the addition operation is performed.
The problem with this solution is that the increase_log_info function is too complicated. How to simplify the increase_log_info function in C language? How parameter types can be mapped directly, rather than through an if...else condition.
Thanks in advance.
Note:
- T_LOG_INFO is data structure definition and cannot be changed.
- LOG_INFO_INCREASE is an update parameter interface provided externally and cannot be changed.
- Other codes can be changed.
#pragma once
/********************************can not be change, begin**********************************/
/* T_LOG_INFO is data structure definition and cannot be changed. */
typedef struct
{
unsigned short tx_num;
unsigned int tx_bytes;
unsigned short rx_num;
unsigned int rx_bytes;
unsigned char discard_num;
unsigned int discard_bytes;
// There are many parameters behind, not listed
}T_LOG_INFO;
T_LOG_INFO g_log_info;
/* This macro is called very frequently, and efficiency needs to be considered.
** LOG_INFO_INCREASE is an update parameter interface provided externally and cannot be changed. */
//#define LOG_INFO_INCREASE(para_idx, inc_val)
/********************************can not be change, end**********************************/
/********************************an alternative, begin**********************************/
enum
{
LOG_PARA_IDX_TX_NUM,
LOG_PARA_IDX_TX_BYTES,
LOG_PARA_IDX_RX_NUM,
LOG_PARA_IDX_RX_BYTES,
LOG_PARA_IDX_DISCARD_NUM,
LOG_PARA_IDX_DISCARD_BYTES,
LOG_PARA_IDX_MAX
};
enum
{
DATA_TYPE_U8,
DATA_TYPE_U16,
DATA_TYPE_U32
};
typedef struct
{
/* Indicates the offset of this parameter in the structure. */
unsigned char offset;
/* Indicates the data type of the parameter. */
unsigned char data_type;
}T_PARA_MAPPING;
/* This table can also be calculated during system initialization. */
T_PARA_MAPPING g_para_mapping_table[LOG_PARA_IDX_MAX] =
{
{0, DATA_TYPE_U16}, // LOG_PARA_IDX_TX_NUM
{4, DATA_TYPE_U32}, // LOG_PARA_IDX_TX_BYTES
{8, DATA_TYPE_U16}, // LOG_PARA_IDX_RX_NUM
{12, DATA_TYPE_U32}, // LOG_PARA_IDX_RX_BYTES
{16, DATA_TYPE_U8}, // LOG_PARA_IDX_DISCARD_NUM
{20, DATA_TYPE_U32} // LOG_PARA_IDX_DISCARD_BYTES
};
/* How to simplify the function??? especially to remove the judgment. */
static inline void increase_log_info(unsigned int para_idx, unsigned inc_val)
{
unsigned int data_type = g_para_mapping_table[para_idx].data_type;
/* Get the parameter address and cast it to the corresponding type pointer before adding. */
if (data_type == DATA_TYPE_U8)
{
*((unsigned char*)(((unsigned char*)&g_log_info) g_para_mapping_table[para_idx].offset)) = inc_val;
}
else if (data_type == DATA_TYPE_U16)
{
*((unsigned short*)(((unsigned char*)&g_log_info) g_para_mapping_table[para_idx].offset)) = inc_val;
}
else
{
*((unsigned int*)(((unsigned char*)&g_log_info) g_para_mapping_table[para_idx].offset)) = inc_val;
}
}
/* This macro is called very frequently, and efficiency needs to be considered. */
#define LOG_INFO_INCREASE(para_idx, inc_val) increase_log_info(para_idx, inc_val)
/********************************an alternative, end**********************************/
/********************************test case, begin**********************************/
void increase_log_info_test()
{
LOG_INFO_INCREASE(LOG_PARA_IDX_TX_NUM, 1);
LOG_INFO_INCREASE(LOG_PARA_IDX_TX_NUM, 2);
LOG_INFO_INCREASE(LOG_PARA_IDX_TX_NUM, 3);
LOG_INFO_INCREASE(LOG_PARA_IDX_RX_BYTES, 10);
LOG_INFO_INCREASE(LOG_PARA_IDX_RX_BYTES, 20);
LOG_INFO_INCREASE(LOG_PARA_IDX_RX_BYTES, 30);
}
/********************************test case, end**********************************/
CodePudding user response:
Basically, you can't. Data types in C are a purely static, compile-time construct. There's no such thing as a variable that holds a type or anything like that.
So you fundamentally can't avoid a chain of if
s, or else a switch
, with the code for each different type written out separately. In principle you can avoid some of the repetition using macros, but that may actually end up being harder to read and understand.
The efficiency isn't so bad, though. A modern compiler is likely to handle an if
chain in an efficient way, and switch
might be even better.
Given this, your array of offsets and types may be unnecessary complexity. I would start with something much simpler, albeit longer:
void increase_log_info(unsigned int para_idx, unsigned inc_val) {
switch (para_idx) {
case LOG_PARA_IDX_TX_NUM:
g_log_info.tx_num = inc_val;
break;
case LOG_PARA_IDX_TX_BYTES:
g_log_info.tx_bytes = inc_val;
break;
// ...
}
}
It'll probably compile into a jump table. That's probably more efficient than what you have, as we don't have to keep accessing the mapping table somewhere else in memory and doing the corresponding address calculations. If it really can be proved to be too slow, you could consider some alternatives, but don't optimize prematurely!
This also has the advantage of being robust if the offset or types of any of the g_log_info
members changes. In your code, you have to remember to manually update your mapping table, or else face very confusing bugs which the compiler will give you no help in detecting.
If you have an extremely large number of members, consider generating this function's C code with a script instead of by hand.
If this is inlined and called with a constant para_idx
value, you can expect the compiler to propagate the constant and emit only the code to update the specific member in question.
CodePudding user response:
Quick answer with, maybe, syntax errors. But I hope the idea can be grasped.
I would prepare an array with a "datatype" of every member of T_LOG_INFO:
{
unsigned short tx_num;
unsigned int tx_bytes;
unsigned short rx_num;
...
}
Copy/paste the above struct and, with a lot of editing, the array would be declared like this:
const char datatypes[LOG_PARA_IDX_MAX] = {
/* unsigned short tx_num; */ 2,
/* unsigned int tx_bytes; */ 4,
/* unsigned short rx_num; */ 4,
...
}
For lazyness, I used numbers like 2, 4 and so on. They indicate mainly the length, but they can carry other info (20=array of 20 char...).
Then I would declare another data structure (the final one):
struct datadesc {
int addr;
char kind;
} datadesc_array[LOG_PARA_IDX_MAX];
Prepare the table in code (the program itself) with:
address=&g_log_info;
for (int i=0; i<LOG_PARA_IDX_MAX; i ) {
datadesc_array[i].addr = address;
datadesc_array[i].kind = datatypes[i];
address = datatypes[i];
}
At this point, when you receive a command, you do:
param_addr = datadesc[para_idx].addr;
param_kind = datadesc[para_idx].kind;
switch (param_kind) {
case 2: // unsigned short
*(short*) param_addr = ...
break;
case 4: // unsigned int
*(unsigned int*) param_addr = ...
}
This way you have a reduced set of cases, just one for every data type you cope with. The only long work is done while preparing the datatypes[] array,