5
\$\begingroup\$

This review is presented in 3 questions due to the amount of code:

  1. Part A contains the Lexical Analyzer and the main portion of the unit test code.
  2. Part B contains the lower level unit tests called in Part A
  3. Part C (this question) contains common unit test code that is included in all of the unit tests to be written.

The Need for Common Unit Test Code

Since I am planning at least 5 unit tests and possibly up to 7 there is a real need to have common code that the unit tests can share, especially unit test logging and error reporting. The functions provided here are used extensively in the unit test code in the other questions.

Questions

I learned C a long time ago from K&R “The C Programming Language” Version 1 (pre C89/C90).

  1. Other than compiling this –O3 what can I do to optimize this code?
  2. Is there archaic C usage that is not customary to use anymore?
  3. Is the code readable?

Code Available:

Rather than copy and pasting this code it is available in my GitHub Repository. The code as presented in these 3 questions is on the branch Before_First_Code_Review, updates including those based on the review will be added to the master branch.

The repository structure. visual map of the GitHub repository.

common_unit_test_logic.h

#ifndef COMMON_UNIT_TEST_LOGIC_H
#define COMMON_UNIT_TEST_LOGIC_H
#include <stdio.h>
#include <stdbool.h>
#ifndef REDUCED_VM_AND_HRF_DEPENDENCIES
#include "human_readable_program_format.h"
#endif

typedef struct test_log_data
{
    char* function_name;
    bool status;
    char* path;
    bool stand_alone;
} Test_Log_Data;

extern FILE* error_out_file;
extern FILE* unit_test_log_file;

extern bool init_vm_error_reporting(char* error_log_file_name);
#ifndef REDUCED_VM_AND_HRF_DEPENDENCIES
extern Human_Readable_Program_Format* default_program(size_t* program_size);
#endif
extern void disengage_error_reporting(void);
extern bool init_unit_tests(char* log_file_name);
extern void report_error_generic(char* error_message);
extern void report_create_and_init_test_log_data_memory_failure(char* function_name);
extern void log_test_status_each_step(char* function_name, bool status, char* path, bool stand_alone);
extern void init_test_log_data(Test_Log_Data* log_data, char* function_name, bool status, char* path, bool stand_alone);
extern Test_Log_Data* create_and_init_test_log_data(char* function_name, bool status, char* path, bool stand_alone);
extern void log_test_status_each_step2(Test_Log_Data* test_data_to_log);
extern void log_start_positive_path(char* function_name);
extern void log_start_positive_path2(Test_Log_Data* log_data);
extern void log_start_test_path(Test_Log_Data* log_data);
extern void log_end_test_path(Test_Log_Data* log_data);
extern void log_end_positive_path(char* function_name);
extern void log_end_positive_path2(Test_Log_Data* log_data);
extern void log_start_negative_path(char* function_name);
extern void log_end_negative_path(char* function_name);
extern void log_generic_message(char *log_message);
extern void close_unit_tests(void);

#endif // !COMMON_UNIT_TEST_LOGIC_H

common_unit_test_logic.c

#include "common_unit_test_logic.h"
#ifndef REDUCED_VM_AND_HRF_DEPENDENCIES
#include "virtual_machine.h"
#endif
#include <ctype.h>
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

FILE* error_out_file = NULL;
FILE* unit_test_log_file = NULL;

bool init_vm_error_reporting(char* error_log_file_name)
{
    bool status_is_good = true;

    if (error_log_file_name)
    {
        error_out_file = fopen(error_log_file_name, "w");
        if (!error_out_file)
        {
            error_out_file = stderr;
            fprintf(error_out_file, "Can't open error output file, %s", "error_log_file_name");
            status_is_good = false;
        }
    }
    else
    {
        error_out_file = stderr;
    }

    return status_is_good;
}

void disengage_error_reporting(void)
{
    if (error_out_file != stderr)
    {
        fclose(error_out_file);
    }
}

bool init_unit_tests(char* log_file_name)
{
    if (log_file_name)
    {
        unit_test_log_file = fopen(log_file_name, "w");
        if (!unit_test_log_file)
        {
            fprintf(error_out_file, "Can't open %s for output\n", log_file_name);
            return false;
        }
        error_out_file = unit_test_log_file;
    }
    else
    {
        unit_test_log_file = stdout;
        error_out_file = stderr;
    }

    return true;
}

void report_error_generic(char *error_message)
{
    fprintf(error_out_file, "%s\n", error_message);
}

void close_unit_tests(void)
{
    if (unit_test_log_file != stdout)
    {
        fclose(unit_test_log_file);
    }
}

static bool log_test_is_positive_path(Test_Log_Data* log_data)
{
    bool is_positive = true;

    if (!log_data->path)
    {
        fprintf(error_out_file, "Programmer error: log_data->path is NULL in log_test_is_positive_path()\n");
        return false;
    }

    char* string_to_test = _strdup(log_data->path);
    if (!string_to_test)
    {
        fprintf(error_out_file, "Memory Allocation error: _strdup() failed in log_test_is_positive_path()\n");
        fprintf(error_out_file, "Exiting program.\n");
        exit(EXIT_FAILURE);
    }

    char* stt_ptr = string_to_test;
    while (*stt_ptr)
    {
        *stt_ptr = (char) toupper(*stt_ptr);
        stt_ptr++;
    }

    is_positive = (strcmp(string_to_test, "POSITIVE") == 0);

    return is_positive;
}

void log_test_status_each_step(char* function_name, bool status, char* path, bool stand_alone)
{
    if (stand_alone)
    {
        fprintf(unit_test_log_file, "%s(): %s Path %s\n", function_name, path,
            (status) ? "Passed" : "Failed");
    }
}

void log_test_status_each_step2(Test_Log_Data *test_data_to_log)
{
    if (test_data_to_log->stand_alone)
    {
        fprintf(unit_test_log_file, "%s(): %s Path %s\n", test_data_to_log->function_name,
            test_data_to_log->path, (test_data_to_log->status) ? "Passed" : "Failed");
    }
}

void log_start_positive_path(char* function_name)
{
    fprintf(unit_test_log_file, "\nStarting POSITIVE PATH testing for %s\n\n",
        function_name);
}

void log_start_positive_path2(Test_Log_Data *log_data)
{
    fprintf(unit_test_log_file, "\nStarting POSITIVE PATH testing for %s\n\n",
        log_data->function_name);
}

void log_end_positive_path(char* function_name)
{
    fprintf(unit_test_log_file, "\nEnding POSITIVE PATH testing for %s\n", function_name);
}

void log_end_positive_path2(Test_Log_Data* log_data)
{
    fprintf(unit_test_log_file, "\nEnding POSITIVE PATH testing for %s, POSITIVE PATH  %s \n",
        log_data->function_name, log_data->status? "PASSED" : "FAILED");
}

void log_start_negative_path(char* function_name)
{
    fprintf(unit_test_log_file, "\nStarting NEGATIVE PATH testing for %s\n\n", function_name);
}

void log_end_negative_path(char* function_name)
{
    fprintf(unit_test_log_file, "\nEnding NEGATIVE PATH testing for %s\n", function_name);
    fflush(unit_test_log_file);        // Current unit test is done flush the output.
}

void log_start_test_path(Test_Log_Data* log_data)
{
    bool is_positive = log_test_is_positive_path(log_data);

    fprintf(unit_test_log_file, "\nStarting %s PATH testing for %s\n\n",
        is_positive ? "POSITIVE" : "NEGATIVE", log_data->function_name);
}

void log_end_test_path(Test_Log_Data *log_data)
{
    bool is_positive = log_test_is_positive_path(log_data);

    fprintf(unit_test_log_file, "\nEnding %s PATH testing for %s, Path %s\n",
        is_positive ? "POSITIVE" : "NEGATIVE", log_data->function_name,
        log_data->status ? "PASSED" : "FAILED");

    if (!is_positive)
    {
        fflush(unit_test_log_file);        // Current unit test is done flush the output.
    }
}

void log_generic_message(char* log_message)
{
    fprintf(unit_test_log_file, log_message);
}

void init_test_log_data(Test_Log_Data* log_data, char *function_name, bool status, char *path, bool stand_alone)
{
    log_data->function_name = function_name;
    log_data->status = status;
    log_data->path = path;
    log_data->stand_alone = stand_alone;
}

Test_Log_Data *create_and_init_test_log_data(char* function_name, bool status, char* path, bool stand_alone)
{
    Test_Log_Data* log_data = calloc(1, sizeof(*log_data));
    if (log_data)
    {
        init_test_log_data(log_data, function_name, status, path, stand_alone);
    }
    else
    {
        fprintf(error_out_file, "In %s calloc() failed\n", "create_and_init_test_log_data");
    }

    return log_data;
}

// provides common error report for memory allocation error.
void report_create_and_init_test_log_data_memory_failure(char *function_name)
{
    fprintf(error_out_file, "In function %s, Memory allocation failed in create_and_init_test_log_data\n", function_name);
}
\$\endgroup\$
2
\$\begingroup\$

typedef struct can reuse the name of struct

I see you often do something like:

typedef struct foo_bar {
    ...
} Foo_Bar;

It's a bit weird to use lower_case for the struct name and Upper_Case for the typedef. You can reuse the same name as the struct:

typedef struct foo_bar {
    ...
} foo_bar;

It's also common to append _t to the typedef'ed name so it is easier to identify it as a type name instead of a variable or function name, although the _t suffix is reserved by at least POSIX 1003.1.

No need to use extern for function declarations

The keyword extern is only necessary to declare variables without defining them, for function declarations there is no need, you can write for example the following in a header file:

bool init_vm_error_reporting(char* error_log_file_name);

Use const where appropriate

It seems like you avoided using const everywhere. Using it might allow the compiler to better optimize your code, and it will be able to report an error if you ever accidentily do write to a variable that shouldn't be changed. So for example:

bool init_vm_error_reporting(const char* error_log_file_name);

You can also use it for struct members:

typedef struct test_log_data
{
    const char* function_name;
    bool status;
    const char* path;
    bool stand_alone;
} test_log_data;

Optimize struct layout

The C standard mandates that the members of a struct appear in the same order in memory as they are declared. But this can result into gaps because of alignment restrictions. The above struct can be better layed out as follows:

typedef struct test_log_data
{
    const char* function_name;
    const char* path;
    bool status;
    bool stand_alone;
} test_log_data;

This saves 8 bytes on 64-bit architectures. In this particular case, it probably won't have a significant impact, but if structs get larger or you use a lot of them, you will reduce the amount of memory (bandwidth) used, and will less likely cause cache misses.

You can close stderr and stdout

It is perfectly fine to call fclose(stdout) and fclose(stderr), so the checks in disengage_error_reporting() and close_unit_tests() are not necessary.

Simplify log_test_is_positive_path()

It looks like you can replace this whole function with:

static bool log_test_is_positive_path(Test_Log_Data* log_data)
{
    return !strcasecmp(log_data, "POSITIVE");
}

Or if you can't use the POSIX strcasecmp() function, Windows provides _stricmp().

But maybe it is better to ensure the filename itself is always upper case, so you can just use strcmp()?

Avoid spending too much code on error handling unrelated to the unit tests

When there is an error internally in the unit tests, like when allocating memory for some string, don't waste lots of lines of code producing nice error messages and exitting gracefully. I particular like the BSD functions like err() for this, but to stay within the C standard, I recommend handling errors using perror() and abort() like so:

test_log_data *create_and_init_test_log_data(const char* function_name, bool status, const char* path, bool stand_alone)
{
    test_log_data* log_data = calloc(1, sizeof(*log_data));
    if (!log_data)
        perror("calloc()"), abort();

    init_test_log_data(log_data, function_name, status, path, stand_alone);
    return log_data;
}
\$\endgroup\$
10
  • 1
    \$\begingroup\$ You found extern which was the basis for the archaic question, it used to be required for functions as well. I trying to stick to the C Standard rather than POSIX or any windows implementations, I included strdup because it finally made it into the C Standard sometime in the last year. In a unit_test it is probably fine to call exit() or abort(), but I've written device drivers and extended operating systems in other ways where that really isn't a good idea. \$\endgroup\$
    – pacmaninbw
    Aug 29 '20 at 12:35
  • 2
    \$\begingroup\$ Sure, in a device driver or in kernel code, exit() and abort() are not options, but you would have something similar to stop everything in case of an unrecoverable error. Unless you are specifically testing whether your code is robust against out-of-memory conditions, OOM is normally considered unrecoverable, so calling abort() is totally fine. Note that strcmp() is C89, so if you ensure consistent capitalization of the filenames, then you don't need to use or reimplement strcasecmp(). \$\endgroup\$
    – G. Sliepen
    Aug 29 '20 at 13:10
  • 1
    \$\begingroup\$ If I use abort() I think I need to add setjmp to main() and longjmp() to everyplace where the error is possible and then call abort() from main() after doing any clean up. \$\endgroup\$
    – pacmaninbw
    Aug 29 '20 at 13:20
  • 1
    \$\begingroup\$ No, don't try to do cleanup after an unrecoverable error, just abort() immediately. This will terminate the program, and will help a debugger see exactly where the error happened and what state your program was in. Trying to do any cleanup is not helpful, and will make debugging harder. \$\endgroup\$
    – G. Sliepen
    Aug 29 '20 at 13:23
  • \$\begingroup\$ " perfectly fine to call fclose(stdout) and fclose(stderr)," --> except that closes those stream for all other code uses as well. OP conditional is a useful idea. \$\endgroup\$ Aug 30 '20 at 13:57

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.