1
\$\begingroup\$

A link to the pull request in question.

I maintain a C++ DLL / Game Maker library that constructs an environment that allows one to call any game maker script/function from within the C++ dll directly. It does this by placing all DLL function calls in a second thread that then communicates function calls to the original thread at which point game maker executes the calls and sends back any return values.

The main thorn this resulted in was a plethora of wrapper functions and other boilerplate. Every time I added support for a game maker function, I would have to go and write a header declaration, write a function to import the Game Maker function ID, and write the actual function declaration which is a somewhat confusing call to add a function call struct to the thread communications. Similarly there were other wrappers to export data for the ACTUAL DLL calls and then call them in the DLL thread when the message was received.

This was all just background to my problem that my recent code changes have solved. I don’t expect anyone to review that DLL interfacing code (though I am always open to improvements!).

To solve this problem I wrote a program called “dynamicWrapperGeneration.c” that when ran parses various headers that used to contain all that boilerplate but now contain nothing but function declarations. It then generates the needed boilerplate files. This file specifically is the one I would like reviewed.

Normally I wouldn’t post here for a small-ish task like this (not gonna lie at one point I considered posting the entire 3000 line library here at one point), but this executable is the first of its kind for me. I have never written anything like this before, so I don’t even know what to call this thing let alone whether my methods are overly crude and clumsy. At some point this executable will actually be expanded to generate the Game Maker side scripts as well, so I want to make sure I am going down a good path with what I currently made so that way I can start to steer myself in a good direction.

Another thing to mention is that the person who primarily owns the repo I am pull requesting this to knows no C++ whatsoever, so anything that might help to make the code friendlier looking to a beginner to C++ software would be appreciated.

I have done code reviews in the past at previous and current jobs in software engineering with fellow devs on the same project, but I have never done one with random people unfamiliar with a project before. So I apologize in advance if I’m either over or under sharing.

#include <stdio.h>
#include <malloc.h>
#include <string.h>
//rs

int walk_function(FILE* inputFile, FILE* outputFile, FILE* outputFile2)
{
    int began = 0;
    int ended = 0;
    int c = fgetc(inputFile);

    while (!ended && c != EOF)
    {
        if (c == ' ')
        {
            ended = began;
            began = 1;
        }
        c = fgetc(inputFile);
    }

    if (c == EOF)
        return c == EOF;

    began = 0;
    ended = 0;

    char* functionName = (char*)malloc(sizeof(char));
    functionName[0] = 0;
    int functionNameLength = 0;

    while (!ended && c != EOF)
    {
        if (c == '(')
        {
            ended = 1;
        } else
        {
            char* tempFunctionName = (char*)malloc((functionNameLength+2)*sizeof(char));
            strcpy(tempFunctionName, functionName);
            tempFunctionName[functionNameLength] = c;
            tempFunctionName[functionNameLength+1] = 0;
            free(functionName);
            functionName = tempFunctionName;
            functionNameLength++;
        }
        c = fgetc(inputFile);
    }

    fprintf(outputFile, "GMEXPORT double get_%s()\n", functionName);
    fprintf(outputFile, "{\n");
    fprintf(outputFile, "    return (double)(int)%s;\n", functionName);
    fprintf(outputFile, "}\n");

    fprintf(outputFile2, "if (function_to_call == (void*)%s)\n", functionName);
    fprintf(outputFile2, "    function_return_value = %s(", functionName);

    int argumentCount = 0;
    int waitingTillComma = 0;
    ended = 0;

    while (!ended && c != EOF)
    {
        if (c == ')')
        {
            ended = 1;
        } else if (c == 'd' && !waitingTillComma)
        {
            fprintf(outputFile2, "dll_input[%u].number", argumentCount);
            waitingTillComma = 1;
            argumentCount++;
        } else if (c == 'c' && !waitingTillComma)
        {
            fprintf(outputFile2, "dll_input[%u].text", argumentCount);
            waitingTillComma = 1;
            argumentCount++;
        } else if (c == ',' && waitingTillComma)
        {
            fprintf(outputFile2, ", ");
            waitingTillComma = 0;
        }
        c = fgetc(inputFile);
    }

    fprintf(outputFile2, ");\n");
    return c == EOF;
}

int walk_GML_function(FILE* inputFile, FILE* outputFile, FILE* outputFile2)
{
    int ended = 0;
    int functionType = 2;
    int c = fgetc(inputFile);

    while (!ended && c != EOF)
    {
        if (c == ' ')
        {
            ended = 1;
        } else if (c == 'v' && functionType == 2)
        {
            functionType = 0;
        } else if (c == 'd' && functionType == 2)
        {
            functionType = 1;
        }
        c = fgetc(inputFile);
    }

    if (c == EOF)
        return c == EOF;

    ended = 0;

    char* functionName = (char*)malloc(sizeof(char));
    functionName[0] = 0;
    int functionNameLength = 0;

    while (!ended && c != EOF)
    {
        if (c == '(')
        {
            ended = 1;
        } else
        {
            char* tempFunctionName = (char*)malloc((functionNameLength+2)*sizeof(char));
            strcpy(tempFunctionName, functionName);
            tempFunctionName[functionNameLength] = c;
            tempFunctionName[functionNameLength+1] = 0;
            free(functionName);
            functionName = tempFunctionName;
            functionNameLength++;
        }
        c = fgetc(inputFile);
    }

    fprintf(outputFile, "ADD_FUNCTION(%s)\n", functionName);

    int argumentCount = 0;
    int* argumentType = NULL;
    int waitingTillComma = 0;
    ended = 0;

    while (!ended && c != EOF)
    {
        if (c == ')')
        {
            ended = 1;
        } else if (c == 'd' && !waitingTillComma)
        {
            int* argumentTypeTemp = (int*)malloc((argumentCount+1)*sizeof(int));
            memcpy(argumentTypeTemp, argumentType, argumentCount*sizeof(int));
            argumentTypeTemp[argumentCount] = 0;
            free(argumentType);
            argumentType = argumentTypeTemp;
            waitingTillComma = 1;
            argumentCount++;
        } else if (c == 'c' && !waitingTillComma)
        {
            int* argumentTypeTemp = (int*)malloc((argumentCount+1)*sizeof(int));
            memcpy(argumentTypeTemp, argumentType, argumentCount*sizeof(int));
            argumentTypeTemp[argumentCount] = 1;
            free(argumentType);
            argumentType = argumentTypeTemp;
            argumentCount++;
        } else if (c == ',' && waitingTillComma)
        {
            waitingTillComma = 0;
        }

        c = fgetc(inputFile);
    }

    if (functionType == 0)
    {
        fprintf(outputFile2, "void %s(", functionName);
    } else if (functionType == 1)
    {
        fprintf(outputFile2, "double %s(", functionName);
    }

    for (int i = 0; i < argumentCount; i++)
    {
        if (argumentType[i] == 0)
        {
            fprintf(outputFile2, "double input%u", i);
        } else if (argumentType[i] == 1)
        {
            fprintf(outputFile2, "const char* input%u", i);
        }

        if (i+1 < argumentCount)
        {
            fprintf(outputFile2, ", ");
        }
    }

    fprintf(outputFile2, ")\n");
    fprintf(outputFile2, "{\n");

    if (functionType == 0)
    {
        fprintf(outputFile2, "    addDelayedFunctionCall(FP_%s, 0", functionName);
    } else if (functionType == 1)
    {
        fprintf(outputFile2, "    return addDelayedFunctionCall(FP_%s, 1", functionName);
    }

    for (int i = 0; i < argumentCount; i++)
    {
        fprintf(outputFile2, ", input%u", i);
    }

    fprintf(outputFile2, ");\n");
    fprintf(outputFile2, "}\n");
    return c == EOF;
}

void parse_GML_header(FILE* outputFile, char* inputFileName)
{
    FILE* inputFile = fopen(inputFileName, "r");

    while (1)
    {
        if (walk_GML_function(inputFile, outputFile, outputFile))
        {
            return;
        }
    }
}

void parse_GML_libraries()
{
    char* outputFileName = "gameMakerGenLibrary.hpp";
    FILE* outputFile = fopen(outputFileName, "w");

    parse_GML_header(outputFile, "gameMakerFunctions\\3DGraphics\\d3d_model.hpp");
    parse_GML_header(outputFile, "gameMakerFunctions\\3DGraphics\\d3d_primitive.hpp");
    parse_GML_header(outputFile, "gameMakerFunctions\\3DGraphics\\d3d_shape.hpp");
    parse_GML_header(outputFile, "gameMakerFunctions\\3DGraphics\\d3d_transform.hpp");

    parse_GML_header(outputFile, "gameMakerFunctions\\gameGraphics\\fontsAndText.hpp");

    parse_GML_header(outputFile, "gameMakerFunctions\\userInteraction\\mouse.hpp");
    parse_GML_header(outputFile, "gameMakerFunctions\\userInteraction\\keyboard.hpp");
    parse_GML_header(outputFile, "gameMakerFunctions\\userInteraction\\joystick.hpp");

    parse_GML_header(outputFile, "gameMakerFunctions\\gamePlay\\rooms.hpp");
}

int main()
{
    char* inputFileName = "gameLoop.hpp";
    FILE* inputFile = fopen(inputFileName, "r");

    char* outputFileName = "getGameLoop.hpp";
    FILE* outputFile = fopen(outputFileName, "w");

    char* outputFileName2 = "callGameLoop.hpp";
    FILE* outputFile2 = fopen(outputFileName2, "w");

    while (1)
    {
        if (walk_function(inputFile, outputFile, outputFile2))
        {
            break;
        }
    }

    parse_GML_libraries();
}

Example Input

Example GameLoop.hpp:

GMEXPORT double gameLoopInit(char* program_directory);
GMEXPORT double gameLoopStep();
GMEXPORT double gameLoopDraw();

Example gameMakerFunctions\3DGraphics\d3d_model.hpp:

double d3d_model_create();
void d3d_model_destroy(double ind);
void d3d_model_load(double ind, const char* fname);
void d3d_model_draw(double ind, double x, double y, double z, double texid);
void d3d_model_primitive_begin(double ind, double kind);
void d3d_model_vertex_texture(double ind, double x, double y, double z, double xtex, double ytex);
void d3d_model_primitive_end(double ind);

Example Output

Example GetGameLoop.hpp:

GMEXPORT double get_gameLoopInit()
{
    return (double)(int)gameLoopInit;
}
GMEXPORT double get_gameLoopStep()
{
    return (double)(int)gameLoopStep;
}
GMEXPORT double get_gameLoopDraw()
{
    return (double)(int)gameLoopDraw;
}

Example callGameLoop.hpp:

if (function_to_call == (void*)gameLoopInit)
    function_return_value = gameLoopInit(dll_input[0].text);
if (function_to_call == (void*)gameLoopStep)
    function_return_value = gameLoopStep();
if (function_to_call == (void*)gameLoopDraw)
    function_return_value = gameLoopDraw();

Example gameMakerGenLibrary.hpp:

ADD_FUNCTION(d3d_model_create)
double d3d_model_create()
{
    return addDelayedFunctionCall(FP_d3d_model_create, 1);
}
ADD_FUNCTION(d3d_model_destroy)
void d3d_model_destroy(double input0)
{
    addDelayedFunctionCall(FP_d3d_model_destroy, 0, input0);
}
ADD_FUNCTION(d3d_model_load)
void d3d_model_load(double input0, const char* input1, const char* input2)
{
    addDelayedFunctionCall(FP_d3d_model_load, 0, input0, input1, input2);
}
ADD_FUNCTION(d3d_model_draw)
void d3d_model_draw(double input0, double input1, double input2, double input3, double input4)
{
    addDelayedFunctionCall(FP_d3d_model_draw, 0, input0, input1, input2, input3, input4);
}
ADD_FUNCTION(d3d_model_primitive_begin)
void d3d_model_primitive_begin(double input0, double input1)
{
    addDelayedFunctionCall(FP_d3d_model_primitive_begin, 0, input0, input1);
}
ADD_FUNCTION(d3d_model_vertex_texture)
void d3d_model_vertex_texture(double input0, double input1, double input2, double input3, double input4, double input5)
{
    addDelayedFunctionCall(FP_d3d_model_vertex_texture, 0, input0, input1, input2, input3, input4, input5);
}
ADD_FUNCTION(d3d_model_primitive_end)
void d3d_model_primitive_end(double input0)
{
    addDelayedFunctionCall(FP_d3d_model_primitive_end, 0, input0);
}

Assume that any files not included are either empty or nothing but whitespace. I didn't design for files to be omitted, but the other "gameMakerFunctions*.hpp" headers are utilized identically.

\$\endgroup\$
13
  • 1
    \$\begingroup\$ This is not C++. This is C. \$\endgroup\$
    – indi
    Commented Jan 9, 2021 at 6:29
  • 1
    \$\begingroup\$ No, that is not C++. It won’t compile on compilers that don’t allow you to use non-const char pointers for a const char array. (Some do allow that as an extension. GCC does, but warns warning: ISO C++ forbids converting a string constant to 'char*'.) That is C code. \$\endgroup\$
    – indi
    Commented Jan 9, 2021 at 7:28
  • 1
    \$\begingroup\$ You can name the file whatever you want; filenames are irrelevant to a C++ compiler. You can name it dynamicWrapperGeneration.java if you want, and still run it through a C++ compiler. It doesn’t matter what the file name is, what matters is what the code is, and that is not C++. That is C. \$\endgroup\$
    – indi
    Commented Jan 9, 2021 at 7:48
  • 2
    \$\begingroup\$ I can’t make my point any clearer: This. Is. Not. C++. It literally is not C++. Your C++ compiler may have let it pass (because most C++ compilers are also C compilers), but it is not legal C++. (Didn’t you see a message in your compiler output telling you that?) It is legal C. If you want it reviewed, you should be asking for C reviewers, not C++ reviewers. If you really want it reviewed by C++ reviewers… well, you’re not going to like what they have to say. (It’ll probably just be: “throw it all out and rewrite it in C++”.) \$\endgroup\$
    – indi
    Commented Jan 9, 2021 at 7:59
  • 1
    \$\begingroup\$ @G.Sliepen added them. Let me know if you have any further questions. Thank you. :) \$\endgroup\$
    – user64742
    Commented Jan 9, 2021 at 21:56

2 Answers 2

3
\$\begingroup\$

malloc vs. realloc

Rather than allocate, copy, append ... ... consider re-allocate, append.

// char* tempFunctionName = (char*)malloc((functionNameLength+2)*sizeof(char));
// strcpy(tempFunctionName, functionName);
// tempFunctionName[functionNameLength] = c;
// tempFunctionName[functionNameLength+1] = 0;
// free(functionName);
// functionName = tempFunctionName;
// functionNameLength++;

functionName = realloc(functionName, functionNameLength + 2);
functionName[functionNameLength++] = c;
functionName[functionNameLength] = '\0';

// Or better, test allocation success

char* tempFunctionName = realloc(functionName, functionNameLength + 2);
if (tempFunctionName == NULL) Handle_OutOfMemory_TBD_code();
functionName = tempFunctionName;
functionName[functionNameLength++] = c;
functionName[functionNameLength] = '\0';

Consider growing the re-allocation is bigger chunks than 1.


Rather than size by type, size by the referenced variable

// ptr = malloc(n * sizeof(type_of_what_p_points_to));
ptr = malloc(sizeof *ptr * n);

Easier to code right, review and maintain. No need to hunt for the type of object ptr points to.

Starting the multiplication with sizeof *ptr insure at least size_t math is used. Useful with more complex calculations.

In C, the cast is not needed. Code as needed if code is also meant for C++.


Alterative while

Use 1 fgetc()

// int c = fgetc(inputFile);
// while (!ended && c != EOF) {
//    ...
//    c = fgetc(inputFile);
//}

int c;
while ((c = fgetc(inputFile)) != EOF && !ended) {
    ...
}

bool vs. int

bool makes sense here

#include <stdbool.h>

//int began = 0;
//int ended = 0;
bool began = false;
bool ended = false;

Maybe fscanf()

Code looks like it is seeking 2 ' ':

//int began = 0;
//int ended = 0;
//int c = fgetc(inputFile);
//while (!ended && c != EOF) {
//    if (c == ' ') {
//        ended = began;
//        began = 1;
//    }
//    c = fgetc(inputFile);
//}
//if (c == EOF) return c == EOF;

// Replaceable with
//                     vvvvvv 1 or more non-spaces     
if (fscanf(inputFile, "%*[^ ])" == EOF) return EOF;
//                     vvvvv  1 space
if (fscanf(inputFile, "%1[ ]%*[^ ])" == EOF) return EOF;
if (fgetc(inputFile) != ' ') return EOF;
\$\endgroup\$
2
\$\begingroup\$
  • DRY

    The block

      while (!ended && c != EOF)                                                                
      {                                                                                         
          if (c == '(')                                                                         
          {                                                                                     
              ended = 1;                                                                        
          } else                                                                                
          {                                                                                     
              char* tempFunctionName = (char*)malloc((functionNameLength+2)*sizeof(char));      
              strcpy(tempFunctionName, functionName);                                           
              tempFunctionName[functionNameLength] = c;                                         
              tempFunctionName[functionNameLength+1] = 0;                                       
              free(functionName);                                                               
              functionName = tempFunctionName;                                                  
              functionNameLength++;                                                             
          }                                                                 
          c = fgetc(inputFile);
      }                                                                                         
    

    is repeated twice. Factor it out into a function.

  • No naked loops. To augment the above bullet point, every loop implements an important job, which deserves a name. For example, the above block is in fact char * parse_function_name(), isn't it? Similarly, it is quite hard to understand what this block is doing:

      while (!ended && c != EOF)
      {   
          if (c == ' ')
          {
              ended = began;
              began = 1;
          }
          c = fgetc(inputFile);
      }
    

    It looks like it is skipping everything until (and including) the very first space. Why? Is it void parse_and_discard_type_declaration()?

    Ditto for parsing arguments.

  • Right tools for the right job. The code assumes a very rigid source file format. You'd be in much better shape taking a flex/bison route.

  • Misc

    • Do not cast what malloc returns. Doing it may lead to mysterious crashes.
    • Always test what malloc returns.
    • I strongly recommend to realloc, rather than manually copy the contents.
    • Along the same line, grow the allocated size "geometrically" (allocated_size *= 2). This would drastically reduce the number of calls to malloc and strcpy.
    • sizeof(char) is guaranteed to be 1. In any case prefer sizeof(variable), as in sizeof(*tempFunctionName). This avoids a double-maintenance problem: if you'd want to switch to another type (say, wchar_t), there'll be just one place to edit.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.