don't click here

C code review

Discussion in 'Technical Discussion' started by BenoitRen, Apr 9, 2024.

  1. BenoitRen

    BenoitRen

    Tech Member
    566
    270
    63
    I'm looking for constructive criticism on the C code I'm writing for my Sonic & Knuckles Collection C port. Given that several people had opinions on portable C code, I'm hoping to get opinions on my output. :)

    Thoughts I already have:
    • There are lots of magic numbers right now, mostly in object initialisation code. I replace them with a #define where it makes sense, like zone numbers, but I'm not sure what the added value would be to hide all of them with a #define.
    • SPRITE_STATUS_x macros for values in the actfree part of sprite_status are a temporary solution. I'm already in the process of replacing them with named members of structs that are defined as part of a union, per Clownacy's suggestion.
    • Due to macros, multi-level structs, and conversions, some lines are quite long. I don't want to enforce an 80-character limit, but line length should still be reasonable.
    • Local functions and local data structures are defined as static. However, this gives problems for data structures when they need to contain pointers to external objects, as a pointer isn't a constant value.
    If there's a better place to ask this, I'm all ears.
     
  2. MainMemory

    MainMemory

    Kate the Wolf Tech Member
    4,757
    341
    63
    SonLVL
    Are you declaring those static struct variables within a function, or outside of a function?
     
  3. BenoitRen

    BenoitRen

    Tech Member
    566
    270
    63
    The static struct variables are being declared outside of a function.
     
  4. President Zippy

    President Zippy

    Zombies rule Belgium! Member
    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).
     
    Last edited: Apr 10, 2024
  5. MainMemory

    MainMemory

    Kate the Wolf Tech Member
    4,757
    341
    63
    SonLVL
    I don't understand why declaring a variable with static linkage would prevent it from including pointers, that seems totally unrelated.
     
  6. 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"?
     
    Last edited: Apr 10, 2024
  7. BenoitRen

    BenoitRen

    Tech Member
    566
    270
    63
    As I understand it, a pointer isn't a constant. However, the address of something is, so an array should work.

    I was trying something like this:
    Code (C):
    1. extern unsigned char* gost08pat;
    Which gave the error: "initializer element is not constant".

    However, it looks like what I need to use is this:
    Code (C):
    1. extern unsigned char gost08pat[];
    I know what I'm going to do tonight. Also going to implement Zippy's suggestion (which, interestingly, would not have worked for array size constants if I was using C89).
     
  8. President Zippy

    President Zippy

    Zombies rule Belgium! Member
    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.
     
    • Informative Informative x 1
    • List
  9. BenoitRen

    BenoitRen

    Tech Member
    566
    270
    63
    I *am* using C99. If I wasn't, I wouldn't be able to use "flexible arrays", but would have to rely on a hack or a non-standard extension (which I learned about today after realising that the latter is exactly what Sonic CD's C port used).

    The structs I'm talking about are definitions, not declarations.

    I tried to use your suggestion to use const variables for constants instead of #define, but it doesn't work for my use cases. This is because in C those are not compile-time constants (they are in C++). That means that I can't use them in array declarations (which I thought was only a limitation in C89), and can't use them as case values in switch statements.

    On the topic of more recent C standards, apparently nameless structs and unions are a feature of C11. A damn shame, because a nameless structs's members can be used as if they're part of the containing struct. This came in handy when adding sprite_status's alternative "family" members, in that I didn't have to go changing every single previous usage of sprite_status's 'regular' members.
     
  10. 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. ;)
     
    Last edited: Apr 11, 2024
  11. President Zippy

    President Zippy

    Zombies rule Belgium! Member
    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.
     
  12. BenoitRen

    BenoitRen

    Tech Member
    566
    270
    63
    Exactly, and that's why I'm still waiting for you to take a look at my code. Until now you've only reacted to the thoughts I posted.
     
  13. President Zippy

    President Zippy

    Zombies rule Belgium! Member
    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)
     
    Last edited: Apr 14, 2024
  14. 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. :)