Bad Comments

I first came across ThreadX somewhere around 2021, during its stint as Azure RTOS. I guess Microsoft thinks blue is cool or something. Maybe they really want to emulate another blue-themed company. I don’t know. Anyway, it’s 62304-certified and I was working on a class B medical device at the time. While using it, I found a bunch of things that look like this:

tx_block_pool_allocate.c

/**************************************************************************/
/*                                                                        */
/*  FUNCTION                                               RELEASE        */
/*                                                                        */
/*    _tx_block_allocate                                  PORTABLE C      */
/*                                                           6.1          */
/*  AUTHOR                                                                */
/*                                                                        */
/*    William E. Lamie, Microsoft Corporation                             */
/*                                                                        */
/*  DESCRIPTION                                                           */
/*                                                                        */
/*    This function allocates a block from the specified memory block     */
/*    pool.                                                               */
/*                                                                        */
/*  INPUT                                                                 */
/*                                                                        */
/*    pool_ptr                          Pointer to pool control block     */
/*    block_ptr                         Pointer to place allocated block  */
/*                                        pointer                         */
/*    wait_option                       Suspension option                 */
/*                                                                        */
/*  OUTPUT                                                                */
/*                                                                        */
/*    status                            Completion status                 */
/*                                                                        */
/*  CALLS                                                                 */
/*                                                                        */
/*    _tx_thread_system_suspend         Suspend thread                    */
/*    _tx_thread_system_ni_suspend      Non-interruptable suspend thread  */
/*                                                                        */
/*  CALLED BY                                                             */
/*                                                                        */
/*    Application Code                                                    */
/*                                                                        */
/**************************************************************************/
UINT  _tx_block_allocate(TX_BLOCK_POOL *pool_ptr, VOID **block_ptr, ULONG wait_option)

This is bad documentation. Before I get into why, I want to say that this isn’t a cherry-picked example; I just went to ThreadX’s GitHub page and picked the first .c file I found.

First, let’s talk a little bit about what makes good documentation. Good documentation is appropriate for its intended audience. It tells them what they need to know, no more and no less. A well-documented API will tell users how it’s intended to be used, what all the inputs and outputs mean, constraints on those inputs and outputs, and warn them about any potentially unexpected behavior like side effects.

Good documentation does not load irrelevant information into the reader’s brain, wasting valuable cognitive capacity. It also doesn’t duplicate (though may refer to) information that’s easily obtained elsewhere, adding to the maintenance burden with no benefit to the reader.

Placement

So getting into my complaints about this comment block, first I want to talk about where it is. You may have noticed that there’s no ; after the function’s prototype. That’s because it’s not a prototype, but an implementation. This block lives in a .c file, where no competent developer will look for API documentation. Interface documentation belongs in the interface, so let’s look at the header where this function is exposed:

tx_api.h

/* Define block memory pool management function prototypes.  */

UINT        _tx_block_allocate(TX_BLOCK_POOL *pool_ptr, VOID **block_ptr, ULONG wait_option);
UINT        _tx_block_pool_create(TX_BLOCK_POOL *pool_ptr, CHAR *name_ptr, ULONG block_size,
                    VOID *pool_start, ULONG pool_size);

I included a few lines of context, just to show that this is real, but that looks an awful lot like a completely undocumented function to me. Why on Earth would you make your API users go digging through your implementation to find out how to use the API? I don’t mean you, specifically, William E. Lamie, of Microsoft Corporation. This is probably a style imposed by the corporate management drones who don’t understand how software is used. It’s probably not your fault.

Improvement

Now I want to look at what a better version of this documentation would look like. First, let’s rip out all the obviously repeated, useless, unhelpful, and extraneous information. If I wanted to know who wrote the code, I’d use git blame. If I wanted to know what language it’s written in, I’d look at the file name or maybe a project-level readme, or maybe the build system, or maybe my resume[1]. If I wanted to know what release version I was looking at, I’d check whatever method I used to get the code, be it a release package or a source control system.

A better version of this comment block would not mention the fact that it’s a function, the name of the function, the release it came in, the language it’s written in, or the original author of the function. All of this information can be gleaned from more reliable sources either elsewhere in this file or in the source control system.

Here’s how it looks without all that noise:

Slightly less noisy

/**************************************************************************/
/*                                                                        */
/*  DESCRIPTION                                                           */
/*                                                                        */
/*    This function allocates a block from the specified memory block     */
/*    pool.                                                               */
/*                                                                        */
/*  INPUT                                                                 */
/*                                                                        */
/*    pool_ptr                          Pointer to pool control block     */
/*    block_ptr                         Pointer to place allocated block  */
/*                                        pointer                         */
/*    wait_option                       Suspension option                 */
/*                                                                        */
/*  OUTPUT                                                                */
/*                                                                        */
/*    status                            Completion status                 */
/*                                                                        */
/*  CALLS                                                                 */
/*                                                                        */
/*    _tx_thread_system_suspend         Suspend thread                    */
/*    _tx_thread_system_ni_suspend      Non-interruptable suspend thread  */
/*                                                                        */
/*  CALLED BY                                                             */
/*                                                                        */
/*    Application Code                                                    */
/*                                                                        */
/**************************************************************************/
UINT  _tx_block_allocate(TX_BLOCK_POOL *pool_ptr, VOID **block_ptr, ULONG wait_option);

Next up, I want to go after the list of functions called by this function. First, if I wanted to know that information, there are tools I could use to find it. Second, the only reasonable way to maintain a list like this is with such a tool. If you’ve already got a tool to generate the set of functions called by a function, why commit its output to source control? Why not just let people use it? If not, this list is going to go out of date and be worse than useless very quickly.

Then there’s the matter of the comments for the called functions. _tx_thread_system_suspend suspends a thread? Great. I think I might have been able to figure that out from its name. You know what I can’t figure out from its name, though? Why. Why is this block allocator suspending threads? That seems like information that might be useful. Granted, the fact that it does suspend threads is also useful information, so this list of called functions is not as atrocious as telling me that this is a function written in C, but if you’re going to tantalize me with something that might be useful, Mr. Lamie, at least tell me the actually useful bit!

Then there’s the fact that it’s not complete. I took a peek at the implementation (since I was already in the wrong file) and you know what else this function does?

From the function body

/* Disable interrupts to get a block from the pool.  */
TX_DISABLE

That’s right, it disables interrupts[2]! Now, if I were an API user and I wanted to know what implications calling some function had, the fact that it disables interrupts globally while it does its work is exactly the kind of thing I’d hope to learn by reading its documentation.

Let’s also examine this CALLED BY block. Why does that exist? Who’s the intended audience here, API users or maintainers? If I’m a maintainer, I absolutely care about the fact that this is part of the public API. Of course, we could have indicated that by PUTTING IT IN THE HEADER THAT DEFINES THE PUBLIC API. Simply putting the documentation in the right place tells users that functions are there for them to use, not hidden away as part of the implementation.

I’m going to leave adding useful information to the documentation out of scope for today, so our cut-down header now looks like this:

Leaving out the misleadingly useless

/**************************************************************************/
/*                                                                        */
/*  DESCRIPTION                                                           */
/*                                                                        */
/*    This function allocates a block from the specified memory block     */
/*    pool.                                                               */
/*                                                                        */
/*  INPUT                                                                 */
/*                                                                        */
/*    pool_ptr                          Pointer to pool control block     */
/*    block_ptr                         Pointer to place allocated block  */
/*                                        pointer                         */
/*    wait_option                       Suspension option                 */
/*                                                                        */
/*  OUTPUT                                                                */
/*                                                                        */
/*    status                            Completion status                 */
/*                                                                        */
/**************************************************************************/
UINT  _tx_block_allocate(TX_BLOCK_POOL *pool_ptr, VOID **block_ptr, ULONG wait_option);

Content

It’s finally time to get into the guts of the comment block: inputs, outputs, how to use the function. First, let’s look at the prototype itself.

A function prototype

UINT  _tx_block_allocate(TX_BLOCK_POOL *pool_ptr, VOID **block_ptr, ULONG wait_option)

This is actually fine. The function has a sensible name; it’s clearly allocating a block from a pool. Its first argument is the pool, which is probably mutated in the allocation process. It returns a pointer to the allocated block in the block_ptr argument. It also takes some sort of option argument for how it behaves with respect to waiting? I don’t know, maybe that’s about what to do if it can’t get one. The return value is obviously a success/failure status of some sort.

So the questions I have after reading this are:

• How can the function fail?

• What does wait_option do, and what are valid values for it?

• What happens to my block pointer if allocation fails?

Not too bad! One whole argument is entirely obvious, a second is mostly obvious, and a third at least tells me I need to look at the documentation to find out what to do with it. I’d be happier if its return type and wait_option were enums, but this is far from the worst I’ve seen.

Now let’s look at the docs and see what the answers to my questions are.

Useless!

/*  OUTPUT                                                                */
/*                                                                        */
/*    status                            Completion status                 */

So, my first question’s answer is…nowhere to be found. Really wishing you’d used an enumeration as a return type here William.

Well, maybe my second question will be answered in the inputs.

anger++

/*  INPUT                                                                 */
/*                                                                        */
/*    pool_ptr                          Pointer to pool control block     */
/*    block_ptr                         Pointer to place allocated block  */
/*                                        pointer                         */
/*    wait_option                       Suspension option                 */

What the hell, Will? Are you trying to make me angry? While the rest of this mess may be the result of a dysfunctional company style, this is inexcusable. “Suspension option?” What kind of garbage is this? What is block_ptr even doing here? It’s an output! It even says so in the INPUT description! Not, like, explicitly, because that would be too clear, but who’s placing things in pointers if not the function being called, Willie? Who?

Alright. This is fine. There’s still the description. Maybe it can save us.

😭

/*  DESCRIPTION                                                           */
/*                                                                        */
/*    This function allocates a block from the specified memory block     */
/*    pool.                                                               */

Look. Bill. I know that it allocates a block. It’s called block_allocate. I even know it uses the specified memory block pool to do it. What I don’t know is anything else. Help me just a little here. What side effects does it have? How does it behave when it fails? What can cause it to fail? What in the name of Dennis Ritchie is a wait option?

Breathe. It’s just software.

Alright, so if I cut out all the useless, misleading, and redundant information, here’s what’s left of our comment block:

Much improved

/**************************************************************************/
/*                                                                        */
/**************************************************************************/
UINT  _tx_block_allocate(TX_BLOCK_POOL *pool_ptr, VOID **block_ptr, ULONG wait_option);

The comment rectangle is pretty silly; I would have said something earlier but there were other problems to tackle. I think you can figure out yourself what’s really left now, Billiam.

As I said, adding useful information is out of scope. Good documentation is not missing. Writing it is left as an exercize for the reader.

Implementation

With that out of the way, let’s look at the comments in the code itself. I don’t want to reproduce the whole function here; let’s just take the first three as a representative sample.

The beginning of the implementation

    /* Disable interrupts to get a block from the pool.  */
    TX_DISABLE

#ifdef TX_BLOCK_POOL_ENABLE_PERFORMANCE_INFO

    /* Increment the total allocations counter.  */
    _tx_block_pool_performance_allocate_count++;

    /* Increment the number of allocations on this pool.  */
    pool_ptr -> tx_block_pool_performance_allocate_count++;
#endif

Looking at these comments, one at a time:

First, we have TX_DISABLE. I already talked about that earlier but way to repeat the code with a comment there, Bill-O.

Next, we have the insightful observation that ++ is the increment operator. Twice! Amazing! There are 5 lines of actual instructions to a compiler here, accompanied by 6 lines that add zero information to help the reader understand them! Billy L, if I were the reviewer for this code, we would have in-person words.

Concluding

This is supposed to be high quality software. It’s even possible that it is, by metrics like reliability, when used correctly. Of course, the only way to use it correctly is to learn how it works.

Don’t write docs like this. Good documentation is a real, achievable thing. Tell people about constraints on your function’s inputs and outputs, and anything that might be surprising about it.

Footnotes

[1]

Also, why not list the C standard to which the code conforms? “Portable C” is not a standard.

[2]

That comment about how we’re disabling interrupts by using a macro called TX_DISABLE may also seem inane; we’ll get to the body’s comments later, but I just want to pause here to point out that TX_DISABLE (TX is just short for ThreadX) is not the most descriptive of names. I thought, maybe this is just locking the thread so we don’t get kicked out by the scheduler or something. So I went looking for the documentation.

Naturally, the header file where the macro’s defined is part of the port, and it’s not wrapped like our allocation function, so each port defines its own completely undocumented implementation. The only thing I could find that looked like documentation of what the macro actually does was in the readme for the Risc-V64 port. To be fair to the author of that file, it’s actually a pretty good description of what the macro does in that port. To be equally fair to the authors of the rest of ThreadX, it’s total bullshit that there isn’t a header with a line like /* Globally disable all interrupts. */ right before a macro definition.

Previous: Foreign Accents