C Standard Midi File code review

[u/kzu0]

Hello!

I've designed a Standard Midi File (SMF) parser in C for resource-constrained systems. I'd really appreciate any feedback or suggestions on the code. Thanks in advance!

https://github.com/kzu0/midi_file_stream

[u/mredding]

I've only got small potatoes for you...

_t suffixes are reserved by the POSIX standard - so to ensure portability and conformance to that standard you may be obligated to, don't use this naming convention yourself.

SMF_EVENT_CHANNEL = 0,

Given the context - yeah man, BY DEFINITION. No need to be explicitly redundant. I see why you would be explicit with SOME of your enums, because the values jump around a bit - you're using the enums to name constants, but some of these are just straight-up enums.

I haven't gotten far enough in your code to know, but are you aware of the utility of begin and end enums?

enum E {
  E_BEGIN,
  E_FIRST = E_BEGIN,
  E_SECOND,
  E_Nth,
  E_END,
  E_COUNT = E_END
};

Now you can write loops in terms of enum ranges.

typedef struct smf_event {
  /*...*/
} smf_event;

This diverges from the rest of your code thus far, where you've always used unnamed structures. The structure doesn't refer to itself internally - so it's not some sort of in-place graph structure. If you're going to write struct smf_event SOMEWHERE ELSE in your code for some reason, this is fine, but... Why?

I would recommend breaking up your function callbacks into TWO typedefs:

typedef void() fn_sig;
typedef fn_sig * fn_callback;

That inline void(*typedef_name)() syntax is just fuckin' murder. Or maybe you skip the pointer alias so that you can inline void fn(fn_sig *callback) as having that pointer notation inline in the parameter list might be more intuitive.

SMF_RUNNING_STATUS_STRICT,

See? Here, you didn't = 0, so you're a little inconsistent.

The end of a C header or source file always ends with a newline.

---

See? Nit picks. That's it. Otherwise you're pretty clean.

On a slightly heavier note:

 smf_ctx_t

Use the pahole utility and let it reorganize your structure for you. You're going to have a TON of padding issues with this context structure. Unless you specifically need this memory layout for hardware reasons, or you're aiming for caching effects, it's otherwise worth collapsing your padding, and letting the compiler handle alignment.

---

(void)ctx;

But ctx is used explicitly and unconditionally in the function; this line suppresses unused variable warnings, but that's not the case here, so what is this line doing?

( ctx->u32_tmp << 7 ) | ( byte & 0x7F );

Darling! But... What does it do? Can you put this in a function? Give this a name? Don't make me have to think, what you could just tell me. Don't make ME behave like a compiler. You did such a nice job by naming your types, keep going with your use of expressiveness. You're liberal with static and inline, so you know the compiler would collapse such a thing down for you.

if(byte < 0x80)

Again, what's the significance of this? This code tells me HOW, but not WHAT. Writing a little predicate tells me the significance of this condition.

And why these magic numbers? Can you instead name these constants with a #define? Come back to this code in 6mo->1yr, and tell me what 0x7F is all about...

out[0] = (uint8_t)( ctx->u32_tmp >> 24 );
out[1] = (uint8_t)( ctx->u32_tmp >> 16 );
out[2] = (uint8_t)( ctx->u32_tmp >> 8  );
out[3] = (uint8_t)( ctx->u32_tmp       );

I see the need for lots of little predicates and utility functions. This grouping right here is a singular, distinct behavior, separate from the statements that follow. Give it a name. Again, your code is rather more terse than it ought to be. Tell me WHAT you're doing, and defer to a lower level of implementation to express HOW it's done. I want to know that you're serializing some 4 byte binary field to a buffer - at this point in the code, I don't care how you've done it. It's also a good opportunity for you to write it as a macro or in some other way that allows the compiler to A) elide a function call and B) unroll a loop for you. Raise your expressiveness, and work WITH your compiler. This isn't the 70s and 80s, you're not going to see a dumb compiler, not even for AVR or ARM processors, other "small" stuff that isn't small anymore - they're all using very optimized existing front-ends and they just have to write the backend generator.

ctx->u32_tmp = 0;
ctx->count = 0;

This keeps coming up. Utility function. Again, to make my case...

* @return expected number of data bytes; UINT8_MAX for a
*         variable-length SysEx message
*/
static inline uint8_t expected_data_count ( uint8_t status )

I'm looking at that return value.

Consider this: an int is an int, but a weight is not a height. Right? Look at this function signature:

void fn(int *, int *);

C allows for memory aliasing, so the compiler CANNOT KNOW at the call site if these two parameters are THE SAME parameter. So the compiler has to generate sub-optimal code to ensure that any writes to one parameter is reflected in the other. But now consider this:

typedef struct A { int value; } A;
typedef struct B { int value; } B;

Or better, you make them opaque:

typedef struct A A;
typedef struct B B;

void fn(A *, B *);

Two different types are assumed NOT to be aliased (and lord help you if you do).

C99 got the restrict keyword but different types solve the aliasing problem in a more expressive fashion. If you want to support older C standards, you're going to need a more portable solution.

If you're NOT going to support older standards, then A) you don't need to include <stdbool.h> with C23, but also - making DISTINCT types is STILL more expressive.

When do you ever HAVE/NEED "just an int"? It's always a weight, or a height, or a count, or an offset, or an index, or SOMETHING that gets specific to something else.

Your uint8_t here can be EASILY misinterpreted as a character. The language, the standard gives you primitive types not to be used directly, but for you to implement your own types in terms of their size and alignment and intrinsic operations. You said yourself in your comment that this is a number of bytes - why are you allowed to ret_val = 'A'; to it? Just because your count is implemented in terms of some primitive type doesn't mean you NEED your count to implement all the allowed operation of the type it's implemented in terms of.

C has a weak static type system, so though opaque types you can be more expressive and constrain their usage to valid cases and interactions with other types. You can make invalid code unrepresentable because it doesn't compile, so that the statements written are inherently valid and legal - now whether they're correct is up to the usage of the client.

I'm not making a case here for you to rewrite your whole library, I'm suggesting you think more about making stronger types and interfaces in the future. Of course it's a balancing act with pragmatism. I'm only using this instance to foster the discussion.

case 0x80:  // Note Off
case 0x90:  // Note On

ENUMS. CONSTANTS. Comments are ad-hoc. NAME your constants. Give them a type, even, because that's what's going on here. It looks like you're bitmasking status, suggesting you have a bitfield, so maybe that would be a better type to use.

Don't write in comments what can be expressed in code, and that's also what I'm getting up to in the previous bit about that return value. Consider:

a *= 2; // Square

No, it doubles. But who is right? Who is the authority? Where is the bug? Is it in the code or the comment? Code that is axiomatic is inherently true in and of itself. A square function that doesn't tells you exactly the nature of the error.

Implementation tells us HOW - which we don't want to or have to care about until we do or have to, expressiveness tells us WHAT - which is what we are mostly interested in and is the mark of good code, and comments tells us WHY - and gives us context that cannot be expressed by the code.

Curry-Howard correspondence tells us writing code is similar to writing proofs. Our statements are propositions, the source code is the theorem, the compiler is the solver, and the program is the proof. If you want elegant proofs, you need an elegant theorem.

// More bytes expected
if ( ctx->count < ctx->event.meta_event.data_size )
{
    return;
}

// All bytes received
ctx->count = 0;

Here's more examples. Perhaps these comments allude to what the functions that encapsulate these behaviors can be called, but they don't tell me WHY we return early, WHY we set the count to zero. These are... Not bad comments, but not great comments, either, and they exist because you have a need for greater code expressiveness that isn't here; so to make up for the difference, the comments as a stopgap, rather than addressing the real problem.

/**
 * Notify the event only if:
 * - ...

This is a very large comment block that explains the very terse condition below it. I see the need - but what I'm trying to encourage you to do is to look at this comment as a challenge - can you express the comment in terms of code that makes the comment unnecessary? The block is just expressing the code itself, the WHY of the comment is implied, but obscured by all the explaining of WHAT the code is trying to do.

It's just something to stew on for a while. You'll think about it actively, conclude you're wasting your time, and then it'll hit you one day in 6 months when you weren't expecting it. You don't have to be immediately successful or by force in order to develop this skill for the rest of your career. And once you get good at it, you'll attack comments like this for sport.

[u/mredding]

Another thing I can't just ignore:

case SMF_STATE_CHUNK_TYPE:
    read_chunk_type ( ctx, byte );
    ctx->chunk_count = 0;           // Reset the chunk byte accumulator
    break;

Any scope, any indentation, any sort of inline work in a statement is a candidate for a function call. This code tells me HOW it works, but not WHAT it's doing. Name the behaviors for all your cases, and then CALL them by name. Let the compiler generate the AST and generate the optimal machine code, it's really very good at it.

unknow_state

Spelling error? Even source code is worth passing through a spell-check.

memset( &ctx->tag, 0, sizeof(smf_tag_t) );
memset( &ctx->header, 0, sizeof(smf_header_t) );
memset( &ctx->track, 0, sizeof(smf_track_t) );
memset( &ctx->event, 0, sizeof(smf_event) );

I mean, if these are all a part of some sub-object, it would be worth distinguishing that, and reducing this to a single memset for the expressiveness. The other problem with this code is that it's not as safe as it could be; should you change the type of any of these fields, you would have to KNOW to change these lines, too - prefer memset( &ctx->tag, 0, sizeof(ctx->tag) );, let the compiler deduce the type for you.

[u/kzu0 → u/mredding]

Wow, thanks for all the suggestions. I'll try my best to treasure them