Mrthdrb

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 *
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': %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 = 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;

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.