6
\$\begingroup\$

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 %s\n",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;
}
\$\endgroup\$
4
\$\begingroup\$

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': %s\n", 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 *
xfopen(const char *filename, const char *mode) {
    FILE *file_pointer = fopen(filename, mode);
    if (file_pointer == NULL) {
        fprintf(stderr, "Error: failed to open file '%s': %s\n",
                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 = xfopen(argv[1], "rb");
    FILE *output_file = xfopen(argv[2], "w");
    bfgen gen = {output_file};
    bfgen_generate_code(&gen, input_file);
    fclose(output_file);
    fclose(input_file);
    return 0;
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for the review. I forgot to add error check for calloc, so this is why this macro was unused. Why would you remove inline? codereview.stackexchange.com/a/216889/177688 . I names the function get_file_pointer because you are getting file pointer from that function. If I would use x, shouldn't be that xfopen instead of xopen? I will work on my int_to_brainfuck function. In main function input_file should be text I think because I want to read newline as one char (not as two chars e.g. on windows). I will post new code later. \$\endgroup\$ – DeBos Apr 14 '19 at 11:49
  • 1
    \$\begingroup\$ I added edited code to my post. \$\endgroup\$ – DeBos Apr 14 '19 at 12:30
  • \$\begingroup\$ @DeBos99 inline is indeed not required: since the program is a single file (or more generally, all function declarations are visible to the compiler) it will willingly inline them. inline could be used to express intent - saying "this function is speed critical, so it needs to be inlined" - but the compiler will inline (or not inline it) regardless. The only real way to force inlining is using a function attribute, __always_inline__ or similar. \$\endgroup\$ – valiano Apr 14 '19 at 13:39
  • \$\begingroup\$ @DeBos99 good catch with xfopen, I've fixed the function name. \$\endgroup\$ – Roland Illig Apr 14 '19 at 15:26
  • \$\begingroup\$ @valiano you are saying that inline is not required, but program runs faster with it, so I think that it should be there. \$\endgroup\$ – DeBos Apr 14 '19 at 16:04
0
\$\begingroup\$

After the first great review by Roland Illig, I could add a few comments on the second code revision, in order of appearance:

  • #include <string.h> - not required now, and could be removed.

  • define ALLOCATION_ERROR 1:
    Rather than preprocessor macros, consider using a named enum. With an enum, you get better diagnostics and type checking from the compiler, plus the error codes are grouped into one named construct:

    enum ERROR_CODES
    {
      ALLOCATION_ERROR = 1,
      FILE_ERROR = 2,
      OTHER_ERROR = 3
    };
    

    As a bonus, you get rid of the unused macro warning.

  • It's best to avoid single-line if and for blocks, which could cause subtle and hard to find bugs. I.e.:

    for(int i=0;i<n;++i)
        fputc(c,output_file);
    

    is better written as:

    for(int i=0;i<n;++i)
    {
        fputc(c,output_file);
    }
    
  • You could early exit the emit_difference function after the if(difference==0) check. Then, you could reduce the indentation level of the code that follows. This technique comes very handy when you have to check for several conditions before doing the actual work. I.e.,

    if(difference==0)
        fputc('.',output_file);
    else{
        // ...
    }
    

    May turn into:

    if(difference==0)
    {
        fputc('.',output_file);
        return;
    }
    
    // Code of the else block
    
  • Style comment: consider adding newlines between your functions, to create a clear separation between them.

  • inline: I second Ronald in that the inline keyword may be safely removed. Since the program is a single file (and more generally, all function declarations are visible to the compiler), it will willingly inline them.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Always use blocks instead of single statements is oft-disputed advice, though I won't restart that style-war here. \$\endgroup\$ – Deduplicator Apr 14 '19 at 15:16
  • \$\begingroup\$ Thanks for review @valiano. I think that using macros for errors is sometimes better e.g. Roland got this warning about unused macro, so I could remove it. Should I add return; at the end of else block or only in if? \$\endgroup\$ – DeBos Apr 14 '19 at 16:09
  • \$\begingroup\$ @DeBos99 no need for a return at the end of the else block, since the function is to return anyway right afterwards. \$\endgroup\$ – valiano Apr 14 '19 at 17:09

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.