C code review

Are you declaring those static struct variables within a function, or outside of a function?

 

Please don't use preprocessor macros to hold constants. Using const variables will make your debugger sessions much more informative at zero cost to performance; constant folding is the simplest optimization a compiler can do, and it's turned on at every optimization level (i.e. O1-O4, and all the specialized optimization modes targeting either low CPU, low RAM, or small executable size).

 

I don't understand why declaring a variable with static linkage would prevent it from including pointers, that seems totally unrelated.

 

If the static pointer is intended to refer to some dynamically allocated resource then could that pose a problem?

My understanding is that the pointer with static storage duration would be initialised with the value of the external pointer prior to it receiving its true value from a call to something like malloc. Resulting in a duration where usage of the pointer would result in undefined behaviour.

If you're not dealing with dynamically allocated objects, could you instead directly refer to the symbol of the external object with "extern"?

 

I keep forgetting that not every C programmer is using C99.

Regarding your "local data structures", are you talking about a struct definition or the declaration of an instance of a struct as a global variable? Either way, for your own sanity I would recommend avoiding the use of non-const globals altogether because having a piece of data that can be modified from any number of functions any different number of ways (even within the same translation unit) makes static analysis dang-near impossible.

For a game as small as S3&K, you could get away with doing nearly everything on the stack, so I would recommend avoid using globals and the heap data altogether. The nice thing about a "pure function" is that given the same arguments, it returns the exact same thing every time. Other than the small amount of I/O you do writing to a GPU framebuffer, playing back MIDI and WAV audio, and reading/writing a save file, nearly all your variables and all your buffers could reside on the stack, and LLVM could statically analyze and report back just about every kind of error you would encounter at runtime.

*On a tangential note, I've argued for "just use C99" in other threads (as opposed to C >= 11 or C89) on the basis that it's a big improvement over its predecessor, but that later versions do not offer any significant improvement. However, the strongest reason to use C99 is that every version of C++ since C++03 designed its C interoperability around C99.

 

Is this a single threaded application? If so, I also presume that the line count would settle within the tens of thousande rather than hundreds. If these are the case, and you're not too concerned with static code analysis, the absence of side effects, or anything else that would be essential to safety critical software, then I'd say go nuts and use global variables.

Just be prepared to pull out your debugger from time to time.

 

1. It costs absolutely nothing to keep variables defined in the function where they are first referenced.

2. One does not simply "pull out the debugger" for a graphical application or one that relies on user input. Unless you have 4 hands, you can't just run a game through LLDB or the VS Debugger and manipulate breakpoints while trying to control the game.

3. Speaking of which, using a debugger on performance-sensitive code and any code that uses timers can alter the behavior of the program, making it impossible to reproduce a bug inside a debugger session. This is what's known as a "Heisenbug". I fixed a Heisenbug exactly once in my 14 years of writing code, and spending an entire week identifying the 2 lines I had to change was the worst experience of my life.

It takes a lot less time and effort to do something right the first time than it does to do it slipshod and then try to fix it later.

 

1. I thought you were going to follow up with one of us by sending a PM asking to review a specific changelist. I didn't know you meant reviewing the entire codebase in one shot. I didn't think this because dropping thousands lines of code on somebody and asking them to review your code with sufficient rigor in less than 4 days is impossible. It took longer getting my coworker (people who are on the payroll) to review my implementation of the session layer of RFC 7540 (using nghttp2 as the parser) which was only 2k LOC.

2. Regardless, here is the same code review I would get if I asked my coworkers at my previous job writing distributed computing middleware in C++ to review this:

(a) How did you compile it? I don't see a Makefile or any Visual Studio sln files, let alone CMakeLists.txt and a README.

(b) How did you test it?

(c) There are no comments, which is exacerbated by most of the variable names being esoteric acronyms. At a bare minimum, put a comment at the top of every function basically explaining what it does and what its params and return value represent, and then I'll take another look at it. Any code review you get on this code without comments will either be somebody repeating what I said, or it will just be a rubber stamp code review.​

Note: Before you complain that I'm excessively fastidious about pedantic coding-style minutiae in my CRs, my coworkers and I were on the payroll, this was part of what we got paid to do, this was exactly how we did it, and the company made a net TTM profit for 17+ years straight and went its entire life with no layoffs doing things this way (AFAIK, at least this was true up until the day I flipped them the bird and quit). In the tech sector, a company that turns a profit within the first decade and doesn't layoff employees every 3 years is a unicorn.

P.S. Because you barked demands at me in a public thread instead of politely asking me in a PM to expedite my VOLUNTEER code review, the next time you want something from me, unless you apologize it will be for a $350/hr consulting fee with a minimum 8hr booking.

EDIT: Recommendation for using CMake (CMakeLists.txt blurb)

 

Personally, I feel that there's overhead attached in striving for high code quality that I'd rather not pursue to any great extent when, for personal projects, I often just want to build something and observe results.

In hindsight, this thread is in request of a code review, and it would appear that @BenoitRen is pursuing a higher standard of quality. So I will consider my previous advice as misplaced and wish them the best of luck in their project.