Search code examples
c++macoscoding-style

Is it a bad practice to use #ifdef in code?


I have to use lot of #ifdef i386 and x86_64 for architecture specific code and some times #ifdef MAC or #ifdef WIN32... so on for platform specific code.

We have to keep the common code base and portable.

But we have to follow the guideline that use of #ifdef is strict no. I dont understand why?

As a extension to this question I would also like to understand when to use #ifdef ?

For example, dlopen() cannot open 32 bit binary while running from 64 bit process and vice versa. Thus its more architecture specific. Can we use #ifdef in such situation?


Solution

  • With #ifdef instead of writing portable code, you're still writing multiple pieces of platform-specific code. Unfortunately, in many (most?) cases, you quickly end up with a nearly impenetrable mixture of portable and platform-specific code.

    You also frequently get #ifdef being used for purposes other than portability (defining what "version" of the code to produce, such as what level of self-diagnostics will be included). Unfortunately, the two often interact, and get intertwined. For example, somebody porting some code to MacOS decides that it needs better error reporting, which he adds -- but makes it specific to MacOS. Later, somebody else decides that the better error reporting would be awfully useful on Windows, so he enables that code by automatically #defineing MACOS if WIN32 is defined -- but then adds "just a couple more" #ifdef WIN32 to exclude some code that really is MacOS specific when Win32 is defined. Of course, we also add in the fact that MacOS is based on BSD Unix, so when MACOS is defined, it automatically defines BSD_44 as well -- but (again) turns around and excludes some BSD "stuff" when compiling for MacOS.

    This quickly degenerates into code like the following example (taken from #ifdef Considered Harmful):

    #ifdef SYSLOG
    #ifdef BSD_42
    openlog("nntpxfer", LOG_PID);
    #else
    openlog("nntpxfer", LOG_PID, SYSLOG);
    #endif
    #endif
    #ifdef DBM
    if (dbminit(HISTORY_FILE) < 0)
    {
    #ifdef SYSLOG
        syslog(LOG_ERR,"couldn’t open history file: %m");
    #else
        perror("nntpxfer: couldn’t open history file");
    #endif
        exit(1);
    }
    #endif
    #ifdef NDBM
    if ((db = dbm_open(HISTORY_FILE, O_RDONLY, 0)) == NULL)
    {
    #ifdef SYSLOG
        syslog(LOG_ERR,"couldn’t open history file: %m");
    #else
        perror("nntpxfer: couldn’t open history file");
    #endif
        exit(1);
    }
    #endif
    if ((server = get_tcp_conn(argv[1],"nntp")) < 0)
    {
    #ifdef SYSLOG
        syslog(LOG_ERR,"could not open socket: %m");
    #else
        perror("nntpxfer: could not open socket");
    #endif
        exit(1);
    }
    if ((rd_fp = fdopen(server,"r")) == (FILE *) 0){
    #ifdef SYSLOG
        syslog(LOG_ERR,"could not fdopen socket: %m");
    #else
        perror("nntpxfer: could not fdopen socket");
    #endif
        exit(1);
    }
    #ifdef SYSLOG
    syslog(LOG_DEBUG,"connected to nntp server at %s", argv[1]);
    #endif
    #ifdef DEBUG
    printf("connected to nntp server at %s\n", argv[1]);
    #endif
    /*
    * ok, at this point we’re connected to the nntp daemon
    * at the distant host.
    */
    

    This is a fairly small example with only a few macros involved, yet reading the code is already painful. I've personally seen (and had to deal with) much worse in real code. Here the code is ugly and painful to read, but it's still fairly easy to figure out which code will be used under what circumstances. In many cases, you end up with much more complex structures.

    To give a concrete example of how I'd prefer to see that written, I'd do something like this:

    if (!open_history(HISTORY_FILE)) {
        logerr(LOG_ERR, "couldn't open history file");
        exit(1);
    }
    
    if ((server = get_nntp_connection(server)) == NULL) {
        logerr(LOG_ERR, "couldn't open socket");
        exit(1);
    }
    
    logerr(LOG_DEBUG, "connected to server %s", argv[1]);
    

    In such a case, it's possible that our definition of logerr would be a macro instead of an actual function. It might be sufficiently trivial that it would make sense to have a header with something like:

    #ifdef SYSLOG
        #define logerr(level, msg, ...) /* ... */
    #else
        enum {LOG_DEBUG, LOG_ERR};
        #define logerr(level, msg, ...) /* ... */
    #endif
    

    [for the moment, assuming a preprocessor that can/will handle variadic macros]

    Given your supervisor's attitude, even that may not be acceptable. If so, that's fine. Instead a macro, implement that capability in a function instead. Isolate each implementation of the function(s) in its own source file and build the files appropriate to the target. If you have a lot of platform-specific code, you usually want to isolate it into a directory of its own, quite possibly with its own makefile1, and have a top-level makefile that just picks which other makefiles to invoke based on the specified target.


    1. Some people prefer not to do this. I'm not really arguing one way or the other about how to structure makefiles, just noting that it's a possibility some people find/consider useful.