0

I have a codebase already containing repetitive code, with only minor differences, serializable ID-s, indexes, variable arrays.

The codebase is huge, and some components are being activated/deactivated based on simple preprocessor directives and constants(e.g.: #define CFG_PROJECT cfgAutobot, #define CFG_PROJECT cfgUltron, ..etc).

The functionality is effectively the same, but with varying components and conditionals. Example:

int somedata;
int somecounter;

void main_loop(){
    #if(CFG_PROJECT == cfgAutobot)
        if(someInterface() == 1){
            somedata = some_other_interface();
        }
    #endif

    #if(CFG_PROJECT == cfgUltron)
        if(third_if() > 0){
            someCounter++;
        }
        else
        {
            someCounter = 0;
        }
    #endif
}

void query_data(int selector){
    if(False){
        /* Dummy block */
    }
    #if(CFG_PROJECT == cfgUltron)
        else if(selector == 1){
            return somedata;
        }
    #endif
    #if(CFG_PROJECT == cfgAutobot)
        else if(selector == 2){
            return someCounter;
        }
    #endif
    else{
        return Err_code;
    }
}

Because the data this code works with is much more complicated, than a simple counter and integer, involves multiple components of varying sizes, these code parts are much more complicated. However they can be traced back to a common structure.

I was able to apply the X-list technique as follows:

#define Ultron_implementation X(var_ultron, (someInterface() == 1), update_function_1, selector_id_1)
#define Autobot_implementation X(var_autobot, (third_if() > 0), update_function_2, selector_id_2)

/* (Please note, that this is a simplified example, in the actual
code there are much more components, but the `main_loop`
implementation can be traced back to a few update functions) */
void update_function_1(int var, int selector) {
    if(selector == 1){
        var++;
    }else{
        var = 0;
    }
}


void update_function_2(int var, int selector) {
    if(selector == 1){
        var = some_other_interface();
    }else{
        /* Nothing to do */
    }
}

#define X(var_name,condition,func_name,sel_id) int var_name;
     Ultron_implementation
     Autobot_implementation
#undef X

void main_loop(){

        #define X(var_name,condition,func_name,sel_id) \
        if(condition){ \
            func_name(var_name, true);\
        }else{ \
            func_name(var_name, false);\
        }
            Ultron_implementation
            Autobot_implementation
        #undef X
}

void query_data(int selector){
    if(False){
        /* Dummy block */
    }
        #define X(var_name,condition,func_name,sel_id) \
        else if(selector == sel_id){ \
            return var_name;\
        }
            Ultron_implementation
            Autobot_implementation
        #undef X

    else{
        return Err_code;
    }
}

The problem with this is that in spite of it now being a unified implementation, the introduction of new components still needs copy-paste, and filtering via previously defined constants(i.e.: CFG_PROJECT) is now excluded from the logic.


Is there a way to minimize the need of copy-pasting into various places in the code and to filter based on defined constants (i.e. CFG_PROJECT)?

SiHa
  • 7,830
  • 13
  • 34
  • 43
Dávid Tóth
  • 2,788
  • 1
  • 21
  • 46
  • This code is just simply not well-architected. Use of `ifdef`s all over the place is a sign that additional functionality was bolted-on as an after-thought. You should look at the Linux kernel which achieves an unreal amount of configuration between enabling features to selecting different architectures. Yes, there are `ifdef`s, but not as many as one would expect, and not often in C files. – Jonathon Reinhart Sep 01 '16 at 13:49
  • The #define - configuration is actually something which could not be avoided, it is what a larger codebase depends on. There were some additional functional requests not mentioned here, of course. For a simple example like this something much more simpler is suggested. – Dávid Tóth Sep 01 '16 at 14:19
  • 1
    What does this have to do with MISRA? If you need the code to be MISRA compliant, you can forget all about that ugly macro mess. Also, `#ifdef` compiler switches are far far more readable than "x macros" and other such meta programming nonsense. – Lundin Sep 02 '16 at 07:59
  • I don't agree, I think it is easier to read a 600 lines long code, than a 5800 lines long one. – Dávid Tóth Sep 02 '16 at 12:24
  • As already pointed out, the need to do these obscure pre-processor practices in the first place originates from poor program design. It seems a simple form of inheritance/polymorphism would have been the solution. Such implementations need not be complicated, it can be enough to just to call specific behavior through a function pointer. And again, why the MISRA tag? Function-like macros are banned by MISRA. – Lundin Sep 02 '16 at 13:01
  • Well then, would please enligthen us with an answer? I am really interested to see what kind of polymorphism is allowed in MISRA-C; And thank you for widening my perspective! – Dávid Tóth Sep 04 '16 at 14:19
  • [This](http://stackoverflow.com/questions/13032015/how-to-implement-a-class-in-c/13032531#13032531) is how you design for private encapsulation in C. The same method can be used for inheritance - inherited structs will have a base class pointer. To implement polymorphism, add a function pointer, which in the base class points to one function, and in the inherited class to another function. This doesn't conflict with MISRA-C, they rather recommend this design as per MISRA 2012. Again, I'm not sure why you brought up MISRA-C since neither your original code nor the modified one conforms to it. – Lundin Sep 05 '16 at 05:55

1 Answers1

0

Filtering to predefined contstants at compile time would require the prerocessor directives #if, #ifdef, etc.. but there is no way to use these inside #define statements AFAIK.

However writing these outside the #define statements is totally legitimate.

#if(CFG_PROJECT == cfgAutobot)
    #define Autobot_implementation X(var_autobot, (third_if() > 0), update_function_2, selector_id_1)
#else
    #define Autobot_implementation
#endif

#if(CFG_PROJECT == cfgUltron)
    #define Ultron_implementation X(var_autobot, (third_if() > 0), update_function_2, selector_id_2)
#else
    #define Ultron_implementation
#endif

And the former can be compiled into a list(of sorts)

#define MACRO_LIST \
    Autobot_implementation \
    Ultron_implementation

Depending on the defined constants the elements of MACRO_LIST will either contain the X() function definition (i.e.: implementation), or an empty constant.

In the implementation now the following can be used:

void main_loop(){

        #define X(var_name,condition,func_name,sel_id) \
        if(condition){ \
            func_name(var_name, true);\
        }else{ \
            func_name(var_name, false);\
        }
            MACRO_LIST
        #undef X
}

To sum up the activated components, see how many components are activated and to refer to them in the implementation, the concatenate (##) token can be used in relation with e.g. an enumeration definition. Example:

#define X(var_name,condition,func_name,sel_id) var_name ## index, 
    tyepdef enum{
        MACRO_LIST
        components_end
    }component_index;
#undef X

some_struct COMPONENT_FLAGS[components_end];

Basically any related variable, ID or implementation can be "serialized" this way.

Please note:

This solution makes the code harder to comprehend, maintain and really difficult to debug, but once it have been tested and verified it eliminates error possibilites coming from copypasting. The result will be a much cleaner, much more elegant and much smaller codebase, than the alternative.

It actually decreased development time from 3 months to a few hours in production code.

Community
  • 1
  • 1
Dávid Tóth
  • 2,788
  • 1
  • 21
  • 46