Program that generates brainfuck code that outputs given text The 2019 Stack Overflow Developer Survey Results Are In Announcing the arrival of Valued Associate #679: Cesar Manara Planned maintenance scheduled April 17/18, 2019 at 00:00UTC (8:00pm US/Eastern)Optimizing string replacement program that uses recursionClassifying lexemes in a given C programInterpreting Brainfuck code to C#, then compiling to a .exeC program that reverses a stringBrainfuck code optimization in PythonC program that recovers lost JPEG filesProgram to find megaPrime numbers between 2 given integersDice roll game that generates random numberProgram that prints out the initials of a given nameText to Brainfuck translator

Do warforged have souls?

First use of “packing” as in carrying a gun

Match Roman Numerals

I'm thinking of a number

Segmentation fault output is suppressed when piping stdin into a function. Why?

Finding the path in a graph from A to B then back to A with a minimum of shared edges

does high air pressure throw off wheel balance?

Do working physicists consider Newtonian mechanics to be "falsified"?

What aspect of planet Earth must be changed to prevent the industrial revolution?

The variadic template constructor of my class cannot modify my class members, why is that so?

I could not break this equation. Please help me

What is this lever in Argentinian toilets?

In horse breeding, what is the female equivalent of putting a horse out "to stud"?

Can withdrawing asylum be illegal?

Didn't get enough time to take a Coding Test - what to do now?

Relations between two reciprocal partial derivatives?

How to colour the US map with Yellow, Green, Red and Blue to minimize the number of states with the colour of Green

What can I do if neighbor is blocking my solar panels intentionally?

Did the new image of black hole confirm the general theory of relativity?

Windows 10: How to Lock (not sleep) laptop on lid close?

Would an alien lifeform be able to achieve space travel if lacking in vision?

Make it rain characters

Why is Captain Marvel translated as male in Portugal?

Wolves and sheep



Program that generates brainfuck code that outputs given text



The 2019 Stack Overflow Developer Survey Results Are In
Announcing the arrival of Valued Associate #679: Cesar Manara
Planned maintenance scheduled April 17/18, 2019 at 00:00UTC (8:00pm US/Eastern)Optimizing string replacement program that uses recursionClassifying lexemes in a given C programInterpreting Brainfuck code to C#, then compiling to a .exeC program that reverses a stringBrainfuck code optimization in PythonC program that recovers lost JPEG filesProgram to find megaPrime numbers between 2 given integersDice roll game that generates random numberProgram that prints out the initials of a given nameText to Brainfuck translator



.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 54 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

                    Can not update quote_id field of “quote_item” table magento 2Magento 2.1 - We can't remove the item. (Shopping Cart doesnt allow us to remove items before becomes empty)Add value for custom quote item attribute using REST apiREST API endpoint v1/carts/cartId/items always returns error messageCorrect way to save entries to databaseHow to remove all associated quote objects of a customer completelyMagento 2 - Save value from custom input field to quote_itemGet quote_item data using quote id and product id filter in Magento 2How to set additional data to quote_item table from controller in Magento 2?What is the purpose of additional_data column in quote_item table in magento2Set Custom Price to Quote item magento2 from controller

                    How to solve knockout JS error in Magento 2 Planned maintenance scheduled April 23, 2019 at 23:30 UTC (7:30pm US/Eastern) Announcing the arrival of Valued Associate #679: Cesar Manara Unicorn Meta Zoo #1: Why another podcast?(Magento2) knockout.js:3012 Uncaught ReferenceError: Unable to process bindingUnable to process binding Knockout.js magento 2Cannot read property `scopeLabel` of undefined on Product Detail PageCan't get Customer Data on frontend in Magento 2Magento2 Order Summary - unable to process bindingKO templates are not loading in Magento 2.1 applicationgetting knockout js error magento 2Product grid not load -— Unable to process binding Knockout.js magento 2Product form not loaded in magento2Uncaught ReferenceError: Unable to process binding “if: function()return (isShowLegend()) ” magento 2

                    Nissan Patrol Зміст Перше покоління — 4W60 (1951-1960) | Друге покоління — 60 series (1960-1980) | Третє покоління (1980–2002) | Четверте покоління — Y60 (1987–1998) | П'яте покоління — Y61 (1997–2013) | Шосте покоління — Y62 (2010- ) | Посилання | Зноски | Навігаційне менюОфіційний український сайтТест-драйв Nissan Patrol 2010 7-го поколінняNissan PatrolКак мы тестировали Nissan Patrol 2016рвиправивши або дописавши її