Program that generates brainfuck code that outputs given text





.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty,.everyoneloves__bot-mid-leaderboard:empty{ margin-bottom:0;
}







5












$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 %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;
}









share|improve this question











$endgroup$



















    5












    $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 %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;
    }









    share|improve this question











    $endgroup$















      5












      5








      5





      $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 %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;
      }









      share|improve this question











      $endgroup$




      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;
      }






      c brainfuck compiler






      share|improve this question















      share|improve this question













      share|improve this question




      share|improve this question








      edited 1 hour ago









      200_success

      131k17157422




      131k17157422










      asked 4 hours ago









      DeBos99DeBos99

      1267




      1267






















          1 Answer
          1






          active

          oldest

          votes


















          2












          $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': %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;
          }





          share|improve this answer











          $endgroup$














            Your Answer






            StackExchange.ifUsing("editor", function () {
            StackExchange.using("externalEditor", function () {
            StackExchange.using("snippets", function () {
            StackExchange.snippets.init();
            });
            });
            }, "code-snippets");

            StackExchange.ready(function() {
            var channelOptions = {
            tags: "".split(" "),
            id: "196"
            };
            initTagRenderer("".split(" "), "".split(" "), channelOptions);

            StackExchange.using("externalEditor", function() {
            // Have to fire editor after snippets, if snippets enabled
            if (StackExchange.settings.snippets.snippetsEnabled) {
            StackExchange.using("snippets", function() {
            createEditor();
            });
            }
            else {
            createEditor();
            }
            });

            function createEditor() {
            StackExchange.prepareEditor({
            heartbeatType: 'answer',
            autoActivateHeartbeat: false,
            convertImagesToLinks: false,
            noModals: true,
            showLowRepImageUploadWarning: true,
            reputationToPostImages: null,
            bindNavPrevention: true,
            postfix: "",
            imageUploader: {
            brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
            contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
            allowUrls: true
            },
            onDemand: true,
            discardSelector: ".discard-answer"
            ,immediatelyShowMarkdownHelp:true
            });


            }
            });














            draft saved

            draft discarded


















            StackExchange.ready(
            function () {
            StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f217422%2fprogram-that-generates-brainfuck-code-that-outputs-given-text%23new-answer', 'question_page');
            }
            );

            Post as a guest















            Required, but never shown

























            1 Answer
            1






            active

            oldest

            votes








            1 Answer
            1






            active

            oldest

            votes









            active

            oldest

            votes






            active

            oldest

            votes









            2












            $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': %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;
            }





            share|improve this answer











            $endgroup$


















              2












              $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': %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;
              }





              share|improve this answer











              $endgroup$
















                2












                2








                2





                $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': %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;
                }





                share|improve this answer











                $endgroup$



                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;
                }






                share|improve this answer














                share|improve this answer



                share|improve this answer








                edited 55 mins ago

























                answered 1 hour ago









                Roland IlligRoland Illig

                11.5k11946




                11.5k11946






























                    draft saved

                    draft discarded




















































                    Thanks for contributing an answer to Code Review Stack Exchange!


                    • Please be sure to answer the question. Provide details and share your research!

                    But avoid



                    • Asking for help, clarification, or responding to other answers.

                    • Making statements based on opinion; back them up with references or personal experience.


                    Use MathJax to format equations. MathJax reference.


                    To learn more, see our tips on writing great answers.




                    draft saved


                    draft discarded














                    StackExchange.ready(
                    function () {
                    StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f217422%2fprogram-that-generates-brainfuck-code-that-outputs-given-text%23new-answer', 'question_page');
                    }
                    );

                    Post as a guest















                    Required, but never shown





















































                    Required, but never shown














                    Required, but never shown












                    Required, but never shown







                    Required, but never shown

































                    Required, but never shown














                    Required, but never shown












                    Required, but never shown







                    Required, but never shown







                    Popular posts from this blog

                    Liste der Baudenkmale in Friedland (Mecklenburg)

                    Single-Malt-Whisky

                    Czorneboh