Vrftsjtryk

I created program that generates brainfuck code that outputs given text.
Arguments for the program are input file with the text and output file where code will be generated to.
Here is the code:

#include <stdio.h>

#include <stdlib.h>

#include <string.h>



#define ALLOCATION_ERROR 1

#define FILE_ERROR 2

#define OTHER_ERROR 3



static inline FILE*

get_file_pointer(const char* filename,const char* mode){

    FILE* file_pointer=fopen(filename,mode);

    if(file_pointer==NULL){

        fprintf(stderr,"Error: failed to open file %sn",filename);

        exit(FILE_ERROR);

    }

    return file_pointer;

}

static inline char*

int_to_brainfuck(int difference){

    if(difference==0)

        return ".";

    else{

        char character_in_loop=difference>0?'+':'-';

        difference=difference>0?difference:-difference;

        const unsigned int loop_body_length=17;

        const unsigned int number_of_ones=(unsigned int)(difference%10);

        const unsigned int number_of_tens=(unsigned int)(difference/10);

        char* brainfuck_code=calloc(number_of_tens+loop_body_length+number_of_ones+2,sizeof*brainfuck_code);

        if(number_of_tens>0){

            brainfuck_code[strlen(brainfuck_code)]='>';

            memset(brainfuck_code+strlen(brainfuck_code),'+',number_of_tens);

            strcat(brainfuck_code+strlen(brainfuck_code),"[<");

            memset(brainfuck_code+strlen(brainfuck_code),character_in_loop,10);

            strcat(brainfuck_code+strlen(brainfuck_code),">-]<");

        }

        memset(brainfuck_code+strlen(brainfuck_code),character_in_loop,number_of_ones);

        brainfuck_code[strlen(brainfuck_code)]='.';

        return brainfuck_code;

    }

}

static inline void

generate_code(FILE* input_file,FILE* output_file){

    int current_char,last_char=0;

    while((current_char=fgetc(input_file))!=EOF){

        char* brainfuck_code=int_to_brainfuck(current_char-last_char);

        fputs(brainfuck_code,output_file);

        if(brainfuck_code[0]!='.')

            free(brainfuck_code);

        last_char=current_char;

    }

}

static inline void

parse_args(int argc){

    if(argc!=3){

        puts("Usage: bfgen <input> <output>");

        exit(OTHER_ERROR);

    }

}

int

main(int argc,char** argv){

    parse_args(argc);

    FILE* input_file=get_file_pointer(argv[1],"r");

    FILE* output_file=get_file_pointer(argv[2],"wb");

    generate_code(input_file,output_file);

    fclose(input_file);

    fclose(output_file);

    return 0;

}

I opened your code in CLion. The first thing it marked was:

#define ALLOCATION_ERROR 1

It's unused.

Other than that, there were no warnings, which is already quite good.

Next I compiled your code using both GCC and CLang:

$ gcc -Wall -Wextra -Os -c bfgen.c

$ clang -Wall -Weverything -Os -c bfgen.c

bfgen.c:5:9: warning: macro is not used [-Wunused-macros]

#define ALLOCATION_ERROR 1

        ^

1 warning generated.

That's also good. You prepared your code quite well for this code review by fixing the compiler warnings (if there had been any).

Now to the human part of the code review.

I would remove the inline from the function definitions. Trust the compiler to do the right thing here.

The word get in the function name get_file_pointer makes it sound as if this function had no side effects. This assumption is wrong. The function should have a better name. It is typical for these error-checking wrapper functions to be prefixed with x. You will find many implementations of xmalloc, xrealloc, xopen, and so on in other projects.

In the get_file_pointer function, you should include the kind of error in the fprintf output:

fprintf(stderr, "Error: failed to open file '%s': %sn", filename, strerror(errno));

The function int_to_brainfuck has a bad name. It's clear from the function signature that it takes an int, therefore the function name should rather describe what that int means. It's a difference, but there is no documentation about what differs. After reading the whole code of the function I know that it's the difference between the previous character and the next character. This information should be encoded in the function name.

Calling strlen repeatedly is a waste of time. At each point in the code you know exactly how long the string is, therefore it is more efficient to just store the current end of the brainfuck_code in a pointer and always strcpy to that pointer:

size_t code_len = number_of_tens + loop_body_length + number_of_ones + 2;

char *brainfuck_code = calloc(code_len, sizeof *brainfuck_code);

char *code = brainfuck_code;



if (number_of_tens > 0) {

    *code++ = '>';

    memset(code, '+', number_of_tens);

    code += number_of_tens;

    strcpy(code, "[<");

    code += 3;

    memset(code, character_in_loop, 10);

    code += 10;

    strcpy(code, ">-]<");

    code += 4;

}



memset(code, character_in_loop, number_of_ones);

code += number_of_ones;

*code++ = '.';



assert(brainfuck_code + code_len == code);



return brainfuck_code;

I added the assert at the end because these explicit length calculations can always go wrong. If the compiler is smart enough, it will find out that this assertion always succeeds and may even eliminate it. And in case you forgot a character to change the code later, you will quickly get a crash dump instead of undefined behavior because of a buffer overflow.

I suspected you had already calculated the length wrong because I couldn't find the first character in the formula. Instead, there's a magic number 2 in that formula. You should make the formula correspond to the actual code by writing it like this:

size_t code_len = 1 + number_of_tens + 3 + 10 + 4 number_of_ones + 1;

This allows you to quickly compare it to the code.

Even better would be if you would not need this whole calculation at all. Since you are writing the output to a file anyway, you don't need to allocate the memory yourself. I'm thinking of two functions like these:

typedef struct {

    FILE *out;

} bfgen;



static void bfgen_emit_str(bfgen *gen, const char *code) {

    ...

}



static void bfgen_emit_repeat(bfgen *gen, char code, size_t n) {

    ...

}

Then you can simply write:

static void

bfgen_emit_difference(bfgen *gen, int difference) {

    if (difference == 0) {

        bfgen_emit_str(".");

        return;

    }



    char character_in_loop = difference > 0 ? '+' : '-';

    unsigned int abs_diff = difference > 0 ? difference : -difference;

    unsigned int number_of_tens = abs_diff / 10;



    if (number_of_tens > 0) {

        bfgen_emit_str(gen, ">");

        bfgen_emit_repeat(gen, '+', number_of_tens);

        bfgen_emit_str(gen, "[<");

        bfgen_emit_repeat(gen, character_in_loop, 10);

        bfgen_emit_str(gen, ">-]<");

    }



    bfgen_emit_repeat(gen, character_in_loop, abs_diff % 10);

    bfgen_emit_str(gen, ".");

}

This code looks much clearer. Now that you don't have to think about all this annoying buffer size calculation anymore, it becomes much easier to add new optimizations, such as avoiding the loop when number_of_tens is exactly 1. In the previous version of the code I wouldn't have added this feature out of laziness and fear of breaking things.

Based on this example, you can see that the functions bfgen_emit_str and bfgen_emit_repeat are really useful, and implementing them is easy.

static void bfgen_emit_str(bfgen *gen, const char *code) {

    fputs(code, gen->out);

}



static void bfgen_emit_repeat(bfgen *gen, char code, size_t n) {

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

        fputc(code, gen->out);

    }

}

Looking at bfgen_emit_difference again, that function doesn't do what its name says. It doesn't only emit code to calculate the difference, it also emits code to print the resulting character. It shouldn't do that. The call to bfgen_emit_str(gen, ".") belongs in bfgen_generate_code instead.

It doesn't matter anymore, but your original version of int_to_brainfuck was essentially:

static char *

int_to_brainfuck(...) {

    if (condition) {

        return ".";

    } else {

        return allocated_string;

    }

}

You must never write such a function since the caller cannot know whether they should free the string or not. This leads either to memory leaks or to undefined behavior. You don't want either of these.

In the main function, you should open the input file in binary mode and the output file in text mode. Currently it's the other way round.

The main takeaway from this code review is that it makes sense to define your own data structures and the corresponding functions. Make these functions as easy as possible to use. Free the caller from any unnecessary tasks such as calculating buffer sizes or managing memory, which are really boring and error-prone.

An idea for further work is to make the generated code more efficient by keeping track of the actual memory contents. This can now be easily done in the struct bfgen. Then you can look which memory cell has currently the closest value and use that instead of just using a single memory cell.

The rewritten and restructured code is:

#include <errno.h>

#include <stdio.h>

#include <stdlib.h>

#include <string.h>



#define FILE_ERROR 2

#define OTHER_ERROR 3



static FILE *

xopen(const char *filename, const char *mode) {

    FILE *file_pointer = fopen(filename, mode);

    if (file_pointer == NULL) {

        fprintf(stderr, "Error: failed to open file '%s': %sn",

                filename, strerror(errno));

        exit(FILE_ERROR);

    }

    return file_pointer;

}



typedef struct {

    FILE *out;

} bfgen;



static void

bfgen_emit_str(bfgen *gen, const char *code) {

    fputs(code, gen->out);

}



static void

bfgen_emit_repeat(bfgen *gen, char code, size_t n) {

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

        fputc(code, gen->out);

    }

}



static void

bfgen_emit_difference(bfgen *gen, int difference) {

    if (difference == 0) {

        return;

    }



    char character_in_loop = difference > 0 ? '+' : '-';

    unsigned int abs_diff = difference > 0 ? difference : -difference;

    unsigned int number_of_tens = abs_diff / 10;



    if (number_of_tens > 0) {

        bfgen_emit_str(gen, ">");

        bfgen_emit_repeat(gen, '+', number_of_tens);

        bfgen_emit_str(gen, "[<");

        bfgen_emit_repeat(gen, character_in_loop, 10);

        bfgen_emit_str(gen, ">-]<");

    }



    bfgen_emit_repeat(gen, character_in_loop, abs_diff % 10);

}



static void

bfgen_generate_code(bfgen *gen, FILE *input_file) {

    int current_char, last_char = 0;

    while ((current_char = fgetc(input_file)) != EOF) {

        bfgen_emit_difference(gen, current_char - last_char);

        bfgen_emit_str(gen, ".n");

        last_char = current_char;

    }

}



static void

parse_args(int argc) {

    if (argc != 3) {

        puts("Usage: bfgen <input> <output>");

        exit(OTHER_ERROR);

    }

}



int

main(int argc, char **argv) {

    parse_args(argc);

    FILE *input_file = xopen(argv[1], "rb");

    FILE *output_file = xopen(argv[2], "w");

    bfgen gen = {output_file};

    bfgen_generate_code(&gen, input_file);

    fclose(output_file);

    fclose(input_file);

    return 0;

}