7

I want to reduce the deep nested branching of this C code and I wonder if there is an analysis tool that can make a truth table for the conditions or if I must analyze it manually? I'd like to make the code more readable and less branching. My IDE CLion from JetBrains doesn't say anything about how to perform such a refactoring. Can it be automated? I didn't try use Lint but I could try it.

if (ptr + j) {
    if (*(ptr + j)[0] == '{') {
        keep = true;
    }
    if (testFn(*(ptr + j))) { /* test for last char */
        string[i][j - p] = concat(*pString1, *(ptr + j));
        keep = false;
        free(*pString1);
        goto mylabel;
    }
    if (keep) {
        *pString1 = concat(*pString1, *(ptr + j));
        *pString1 = concat(*pString1, " ");
        p++;
    } else {
        b1 = false;
        int q = j;
        for (e = 0; *(ptr + q + e); e++) { /* step through the string */
            b1 = true;
            if (*(ptr + e + q)) {
                *pString = concat(*pString, *(ptr + e + q));
                *pString = concat(*pString, " ");
            }
            j = e;
        }
        if (makeArgs(*pString, &argc, (const char ***) &argv, pipe, i, h)) {
            write_command(&w, argv, string[w]);
            w++;

        } else {
            if (!b1) { /* no args (?) */
                for (int r = 0; argv[r] != NULL; r++) {
                    string[i][r] = argv[r]; /* is this necessary? */
                }
            }
        }
    }
}

I've been able to reduce the conditional branching manually, but I havn't done a truth table. I think the code analyses should say which branches are identical and when with a truth table.

The complete function looks like this today:

static int runCmd(const char *cmd) {
    const char *cp;
    pid_t pid;
    int status;
    struct command structcommand[15];
    char **argv = 0;
    int argc = 1;
    bool pipe = false;
    char *string[z][z];
    char *pString3[40];
    char *pString2[40];
    int n = 0;
    char **ptr1;
    char string1[z];
    bool keep = false;
    char *pString1[z];
    char *pString[z];
    *pString1 = "\0";
    *pString = "\0";
    char *temp = {'\0'};
    int w = 0;
    bool quote = false;
    int rrs[256];
    int j = 0;
    int i;
    int p = 0;
    char **ptr;
    int count = 0;
    char *cmdtmp;
    bool b1 = false;
    int y = 0;
    i = 0;
    int h = 0;
    char *str;
    char *freeme[75][75];
    char **dealloc[75];
    char **dealloca[75][75];
    int acount[128];
    nullterminate(string);
    int rr = 0;
    for (z = 0; z < 128; z++) {
        acount[z] = -1;

    }
    for (int f = 0; f < 75; f++) {
        dealloc[f] = NULL;
        for (z = 0; z < 75; z++) {
            freeme[f][z] = NULL;
        }
    }
    if (cmd) {
        for (cp = cmd; *cp; cp++) {
            if ((*cp >= 'a') && (*cp <= 'z')) {
                continue;
            }
            if ((*cp >= 'A') && (*cp <= 'Z')) {
                continue;
            }
            if (isDecimal(*cp)) {
                continue;
            }
            if (isBlank(*cp)) {
                continue;
            }
            if ((*cp == '.') || (*cp == '/') || (*cp == '-') ||
                (*cp == '+') || (*cp == '=') || (*cp == '_') ||
                (*cp == ':') || (*cp == ',') || (*cp == '\'') ||
                (*cp == '"')) {
                continue;
            }
        }
        cmdtmp = strdup(cmd);
        ptr1 = str_split(pString3, cmdtmp, '|');
        if (strstr(cmd, "|") == NULL) {         /* not a pipeline */
            makeArgs(cmd, &argc, (const char ***) &argv, pipe, 0, 0);
            write_argument(&argc, structcommand, argv, string[0]);
            n++;
        }
        else {
            for (i = 0; *(ptr1 + i); i++) { /* loop for each pipeline*/
                n++;
                /* save number of pipelines */
                dealloc[n] = NULL;
                int e = 0; /* a counter */
                *pString = "\0"; /* should malloc and free this? */
                strcpy(string1, *(ptr1 + i));
                if ((string1[0] != '\0') &&
                    !isspace(string1[0])) { /* this is neither the end nor a new argument */ /* BSD bug? check*/

                    ptr = str_split(pString2, *(&string1), ' '); /* split the string at the arguments */
                    dealloc[rr] = ptr;
                    rr++;
                    h = 0;
                    for (j = 0; *(ptr + j); j++) { /* step through the arguments */
                        dealloca[n][n - 1] = NULL;
                        /* the pipeline is in cmdtmp and the argument/program is in ptr[i] */
                        if (ptr + j && !quote && strstr(*(ptr + j), "'")) { /* is quote? */
                            quote = true;
                            strcpy(temp, *(ptr + j)); /* point where quoted piipelines crash */
                            if (y < 1) {
                                y++;
                            }
                        }
                        while (quote) {
                            if (*(ptr + j) && strstr(*(ptr + j), "'")) { /* end of quote */
                                quote = false;
                                if (y < 1) {
                                    string[i][j] = strcpy(temp, *(ptr + j));
                                }
                                y = 0;
                            }
                            else if (*(ptr + j)) { /* read until end of quote */
                                string[i][j] = temp;
                                continue;
                            } else {
                                quote = false;
                                break;
                            }
                        }
                        if (ptr + j) { ;
                            if (*(ptr + j)[0] == '{') {
                                keep = true;
                            }
                            if (testFn(*(ptr + j))) { /* test for last char */
                                string[i][j - p] = concat(*pString1, *(ptr + j));
                                keep = false;
                                free(*pString1);
                                continue;//goto mylabel;
                            }
                            if (keep) {
                                str = concat(*pString1, *(ptr + j));
                                *pString1 = concat(str, " ");
                                free(str);
                                p++;
                            } else {
                                b1 = false;
                                int q = j;
                                freeme[i][0] = *pString;
                                for (e = 0; *(ptr + q + e); e++) { /* step through the string */
                                    b1 = true;
                                    if (*(ptr + e + q)) {
                                        str = concat(*pString, *(ptr + e + q));

                                        *pString = concat(str, " "); /* how to free() ? */
                                        free(str);
                                        freeme[i][e] = *pString;



                                    }
                                    j = e; /* adjust the counter */
                                }


                                if (makeArgs(freeme[i][e - 1], &argc, (const char ***) &argv, pipe, i, h)) {
                                    write_command(&w, argv, string[w]);
                                    w++;
                                    for (int qwe = 0; qwe < argc; qwe++) {

                                        dealloca[n - 1][qwe] = &argv[qwe];
                                    }
                                    acount[n - 1] = argc;

                                } else {
                                    if (!b1) { /* no args (?) */
                                        for (int r = 0; argv[r] != NULL; r++) {
                                            string[i][r] = argv[r]; /* is this necessary? */
                                        }
                                    }
                                }
                            }
                        }

                    }

                    bool boo = false;
                    dump_argv((const char *) "d", argc, argv, boo);
                }



            }


        }

        for (i = 0; i < n; i++) {

            structcommand[i].argv = string[i];
            for (j = 0; string[i][j] != NULL; j++) {
                if (string[i] != NULL) {


                }
            }



        }
        free(cmdtmp);
        if (ptr1) {
            int i;
            for (i = 0; *(ptr1 + i); i++) {

                free(*(ptr1 + i));
            }
            printf("\n");
            free(ptr1);
        }

        fflush(NULL);
        pid = fork();
        if (pid < 0) {
            perror("fork failed");
            return -1;
        }
        /* If we are the child process, then go execute the string.*/
        if (pid == 0) {
            /* spawn(cmd);*/
            fork_pipes(n, structcommand);
        }
        /*
         * We are the parent process.
         * Wait for the child to complete.
         */
        status = 0;
        while (((pid = waitpid(pid, &status, 0)) < 0) && (errno == EINTR));
        if (pid < 0) {
            fprintf(stderr, "Error from waitpid: %s", strerror(errno));
            return -1;
        }
        if (WIFSIGNALED(status)) {
            fprintf(stderr, "pid %ld: killed by signal %d\n",
                    (long) pid, WTERMSIG(status));

            return -1;
        }
    }


    for (i = 0; i < n; i++) {

        for (j = 0; string[i][j] != NULL; j++) {
            if (string[i] != NULL) {

                if (string[i][j])
                    free(string[i][j]);

            }
        }

    }

    int z;
    for (int f = 0; f < n; f++) {
        if (f > 0) {

        }
        for (z = 0; freeme[f][z]; z++) {

            free(freeme[f][z]);
        }

    }

    size_t idx;
    for (int f = 0; n > 1 && f < n; f++) {

        for (idx = 0; *(dealloc[f] + idx) != NULL; idx++) {
            free(*(dealloc[f] + idx));
        }
        free(dealloc[f]);

    }

    return WEXITSTATUS(status);


}

The code is scanning and parsing another program, that's why it is so much string manipulation, saving and looking ahead at characters and pointers.

  • 2
    If your code still uses goto - so avoiding the use of keep then you have not done well. – Steve Barnes May 1 '16 at 13:49
  • 2
    Yes it is essential in assembler but considered very bad practice is C due to the issues with a) maintenance issues & b) stack corruption because it is too easy to goto somewhere outside of the function or procedure - not return. That is why it is rare! – Steve Barnes May 1 '16 at 14:07
  • 1
    This code is very strange. I assume "ptr" is declared as "char *". How can "ptr+j" ever be zero? Can you explain what "*(ptr+j)[0]" does? Does it actually compile? – Ira Baxter May 3 '16 at 8:29
  • 1
    OK, so "ptr" is declared "**char". How can "ptr+j" ever be zero? You didn't answer the question about what you explicitly want for "truth tables". (Are all those array sizes really hardwired? This isn't preprocessed code; it still contains comments). (Yes, I know about recursive descent parsers, but I usually don't implement them like this: see stackoverflow.com/questions/2245962/…) – Ira Baxter May 3 '16 at 8:47
  • 1
    Analysis cannot modify your code. Analysis is a read-only operation. Maybe get familiar with some concepts first, e.g. pluralsight.com/courses/brownfield Write unit tests, measure the code coverage or better branch coverage. Then look for an IDE which has refactoring methods like "Extract method" and "Invert if statement", something like Jetbrains Resharper but for C. It does identify logical errors like "This if condition is always true". – Thomas Weller May 3 '16 at 16:04
2

I’ve had some success using a tool called CppCheck via a Jenkins CI system. I don’t specifically track conditional branches, but, the amount of checks this tool provides is worth the look. In particular, check the Condition part which lists various checks for conditions that are always true/false (including value tracking, duplicate conditions, interval logic), but other categories also list some possibly useful checks like:

  • “pointer addition in condition“
  • “identical code in both branches of if/else or ternary operator.”
  • other types of suspicious or redundant conditions (for STL, strings, logic/numeric operations, ...)

And, it’s available as a plugin for your IDE.

  • 1
    “identical code in both branches of if/else or ternary operator.” Wow. That's the most interesting example of what CppCheck can do? I've been coding for 45 years and I've never encountered this. – Ira Baxter May 5 '16 at 7:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.