Good Documentation¶

Tearing apart bad documentation may be cathartic, but there’s good documentation out there too. It deserves recognition. Today, I’m going to pick on SDL. I’m not talking about the quality of the API itself, just its documentation.

In the anti-cherry-picking spirit of the previous post, I’ll take the first file I find. That’s SDL_assert.h since SDL.h is just a bunch of #includes with 0 comments. Not that there’s anything wrong with that.

The first thing I want to say is that I find Doxygen ugly. SDL doesn’t actually use Doxygen, exactly, but they do use a lot of things that look a lot like Doxygen, including /** and \param. I’m holding this API up as an exemplar of good documentation in spite of these things.

That said, if ever there were a use case for generated docs instead of just reading the headers, it would be in a widely used library that’s usually installed on desktop computers and just linked. Most people do not find themselves reading SDL’s headers, much less code. So the annotations that make the generated docs a bit easier to read at the cost of making the headers a bit harder to read are probably on the good end of the tradeoff scale.

File-level¶

So without further ado, here’s the first comment block in the file:

SDL_Assert.h (file-level)¶

/**
 * # CategoryAssert
 *
 * A helpful assertion macro!
 *
 * SDL assertions operate like your usual `assert` macro, but with some added
 * features:
 *
 * - It uses a trick with the `sizeof` operator, so disabled assertions
 *   vaporize out of the compiled code, but variables only referenced in the
 *   assertion won't trigger compiler warnings about being unused.
 * - It is safe to use with a dangling-else: `if (x) SDL_assert(y); else
 *   do_something();`
 * - It works the same everywhere, instead of counting on various platforms'
 *   compiler and C runtime to behave.
 * - It provides multiple levels of assertion (SDL_assert, SDL_assert_release,
 *   SDL_assert_paranoid) instead of a single all-or-nothing option.
 * - It offers a variety of responses when an assertion fails (retry, trigger
 *   the debugger, abort the program, ignore the failure once, ignore it for
 *   the rest of the program's run).
 * - It tries to show the user a dialog by default, if possible, but the app
 *   can provide a callback to handle assertion failures however they like.
 * - It lets failed assertions be retried. Perhaps you had a network failure
 *   and just want to retry the test after plugging your network cable back
 *   in? You can.
 * - It lets the user ignore an assertion failure, if there's a harmless
 *   problem that one can continue past.
 * - It lets the user mark an assertion as ignored for the rest of the
 *   program's run; if there's a harmless problem that keeps popping up.
 * - It provides statistics and data on all failed assertions to the app.
 * - It allows the default assertion handler to be controlled with environment
 *   variables, in case an automated script needs to control it.
 * - It can be used as an aid to Clang's static analysis; it will treat SDL
 *   assertions as universally true (under the assumption that you are serious
 *   about the asserted claims and that your debug builds will detect when
 *   these claims were wrong). This can help the analyzer avoid false
 *   positives.
 *
 * To use it: compile a debug build and just sprinkle around tests to check
 * your code!
 */

So this is a file-level comment, explaining what’s in the file, why you would want to use it, and how to use it. There’s an extra * in the first line, a single comment-comment to tell the document generator about wiki categories, and then zero further characters that are not there for the benefit of a human reader (or required by the compiler).

There’s no \brief nonsense; the short description is on the first line, separated by whitespace, so a person can easily parse it. The bulleted list simultaneously explains why this file duplicates functionality in the standard library and what kinds of things someone might use it to do. It talks about performance implications of using the things in this file, and non-obvious implications of using it (like Clang’s static analysis hints). All-around good documentation.

A Macro¶

The next documented symbol in this file is a constant macro. Amusingly, it’s defined to SomeNumberBasedOnVariousFactors inside the document generator’s context. The specifics of the factors are pretty easily gleaned from the next few lines, but they’re also pretty ugly and environment-dependent.

Documentation for a macro¶

#ifdef SDL_WIKI_DOCUMENTATION_SECTION

/**
 * The level of assertion aggressiveness.
 *
 * This value changes depending on compiler options and other preprocessor
 * defines.
 *
 * It is currently one of the following values, but future SDL releases might
 * add more:
 *
 * - 0: All SDL assertion macros are disabled.
 * - 1: Release settings: SDL_assert disabled, SDL_assert_release enabled.
 * - 2: Debug settings: SDL_assert and SDL_assert_release enabled.
 * - 3: Paranoid settings: All SDL assertion macros enabled, including
 *   SDL_assert_paranoid.
 *
 * \since This macro is available since SDL 3.2.0.
 */
#define SDL_ASSERT_LEVEL SomeNumberBasedOnVariousFactors
#elif !defined(SDL_ASSERT_LEVEL)
#ifdef SDL_DEFAULT_ASSERT_LEVEL
#define SDL_ASSERT_LEVEL SDL_DEFAULT_ASSERT_LEVEL
#elif defined(_DEBUG) || defined(DEBUG) || \
      (defined(__GNUC__) && !defined(__OPTIMIZE__))
#define SDL_ASSERT_LEVEL 2
#else
#define SDL_ASSERT_LEVEL 1
#endif
#endif

So here we have a short description of what the symbol is, a vague description of how its value is managed, a caveat about forward compatibility (including an implicit guarantee that existing values won’t change), and an enumerated list of possible values and their meanings.

Then there’s a tag for the documentation generation system (since) and a statement about what versions of the library support it.

I like how each setting is given a short name and an expanded description. I don’t love this as a way to make an enumeration, but we’re here to talk about the documentation, not the API being documented. And besides, this is intended to be configured in the build system, so this may actually be the cleanest way to do it in portable C.

Another Macro¶

Now we have a macro function: SDL_TriggerBreakpoint(). It looks like a function, it’s named just like the other functions in SDL. Let’s take a look at its documentation.

Macro function¶

/**
 * Attempt to tell an attached debugger to pause.
 *
 * This allows an app to programmatically halt ("break") the debugger as if it
 * had hit a breakpoint, allowing the developer to examine program state, etc.
 *
 * This is a macro--not a function--so that the debugger breaks on the source
 * code line that used SDL_TriggerBreakpoint and not in some random guts of
 * SDL. SDL_assert uses this macro for the same reason.
 *
 * If the program is not running under a debugger, SDL_TriggerBreakpoint will
 * likely terminate the app, possibly without warning. If the current platform
 * isn't supported, this macro is left undefined.
 *
 * \threadsafety It is safe to call this macro from any thread.
 *
 * \since This macro is available since SDL 3.2.0.
 */
#define SDL_TriggerBreakpoint() TriggerABreakpointInAPlatformSpecificManner

Short description of what it does, slightly longer statement about why someone would use it, followed by several things a user should know when calling this function (macro).

Why is it implemented as a macro? “…so that the debugger breaks on the source code line that used SDL_TriggerBreakpoint and not in some random guts of SDL.”

What is the intended use for this? When the program is running under a debugger.

What will happen if it’s used outside that context? Something you probably don’t want your users to encounter.

What happens if it’s used where it’s not supported? Compile error.

Amazing. Then we have some annotation about thread safety and API version support. Useful!

Enumeration¶

After skipping over some more well-documented macros (and a nice /* comment explaining to maintainers why something is the way it is) we come to a typedef enum:

typedef enum¶

/**
 * Possible outcomes from a triggered assertion.
 *
 * When an enabled assertion triggers, it may call the assertion handler
 * (possibly one provided by the app via SDL_SetAssertionHandler), which will
 * return one of these values, possibly after asking the user.
 *
 * Then SDL will respond based on this outcome (loop around to retry the
 * condition, try to break in a debugger, kill the program, or ignore the
 * problem).
 *
 * \since This enum is available since SDL 3.2.0.
 */
typedef enum SDL_AssertState
{
    SDL_ASSERTION_RETRY,  /**< Retry the assert immediately. */
    SDL_ASSERTION_BREAK,  /**< Make the debugger trigger a breakpoint. */
    SDL_ASSERTION_ABORT,  /**< Terminate the program. */
    SDL_ASSERTION_IGNORE,  /**< Ignore the assert. */
    SDL_ASSERTION_ALWAYS_IGNORE  /**< Ignore the assert from now on. */
} SDL_AssertState;

What can I say? It’s got a short description, pointers to related functions, and what all of the values mean. It’s good, it’s parsimonious, and instead of listing something like RETURNED_BY: it suggests to the user why it exists and where to look to find how to use it.

Data structure¶

The next thing to be defined in the file is a struct:

typedef struct¶

/**
 * Information about an assertion failure.
 *
 * This structure is filled in with information about a triggered assertion,
 * used by the assertion handler, then added to the assertion report. This is
 * returned as a linked list from SDL_GetAssertionReport().
 *
 * \since This struct is available since SDL 3.2.0.
 */
typedef struct SDL_AssertData
{
    bool always_ignore;  /**< true if app should always continue when assertion is triggered. */
    unsigned int trigger_count; /**< Number of times this assertion has been triggered. */
    const char *condition;  /**< A string of this assert's test code. */
    const char *filename;  /**< The source file where this assert lives. */
    int linenum;  /**< The line in `filename` where this assert lives. */
    const char *function;  /**< The name of the function where this assert lives. */
    const struct SDL_AssertData *next;  /**< next item in the linked list. */
} SDL_AssertData;

We know what the information is about, what the data structure’s used for, what all the members mean, and that it’s a linked list. The names of the fields are pretty good, so their descriptions are largely redundant, but the boilerplate is minimal and most of the descriptions do actually add information. At least trigger_count isn’t documented as “trigger count.”

Function¶

Finally we come to a function declaration. The first one, SDL_ReportAssertion, says never to call it directly so we’ll skip that. Instead, I want to talk about a real function that’s intended to be used by API users.

SDL_SetAssertionHandler¶

/**
 * Set an application-defined assertion handler.
 *
 * This function allows an application to show its own assertion UI and/or
 * force the response to an assertion failure. If the application doesn't
 * provide this, SDL will try to do the right thing, popping up a
 * system-specific GUI dialog, and probably minimizing any fullscreen windows.
 *
 * This callback may fire from any thread, but it runs wrapped in a mutex, so
 * it will only fire from one thread at a time.
 *
 * This callback is NOT reset to SDL's internal handler upon SDL_Quit()!
 *
 * \param handler the SDL_AssertionHandler function to call when an assertion
 *                fails or NULL for the default handler.
 * \param userdata a pointer that is passed to `handler`.
 *
 * \threadsafety It is safe to call this function from any thread.
 *
 * \since This function is available since SDL 3.2.0.
 *
 * \sa SDL_GetAssertionHandler
 */
extern SDL_DECLSPEC void SDLCALL SDL_SetAssertionHandler(
                                            SDL_AssertionHandler handler,
                                            void *userdata);

So here we’re registering a callback. Why would anyone want this? What happens if the user never calls it? Both answered in the first paragraph.

Are there special things we should know about how handler will be called? Yes! It can fire from any thread, but only from one thread at a time.

Does this function have any surprising behavior? Yes. Its effects are not reversed by calls to SDL_Quit().

Then it actually lists out the parameters. Interesting fact: passing NULL as the handler will reset it to the default.

I could have guessed that userdata was going to get passed to handler, but saying this explicitly is a lot better than \param userdata user data.

We also added a “see also” annotation for the documentation generator. This becomes a link in the wikified docs, but it’s actually pretty useful in the header too. Note that this isn’t a list of functions in the call graph, but related a function that an API user might want to know about.

Comments¶

Normally, headers don’t have much in the way of comments on the implementation. They’re just there to provide an interface, after all. However, a header that provides assertion macros is necessarily going to have some implementation in it. As such, there are comments intended for maintainers. These are kept outside the #ifdef SDL_WIKI_DOCUMENTATION_SECTION blocks, and they begin with /* instead of /**. Here are a couple representative samples:

Regular comment¶

#elif defined(_MSC_VER) && !defined(__clang__)  /* Avoid /W4 warnings. */
/* "while (0,0)" fools Microsoft's compiler's /W4 warning level into thinking
    this condition isn't constant. And looks like an owl's face! */
#define SDL_NULL_WHILE_LOOP_CONDITION (0,0)

This comment explains why we’re doing this weird thing. It’s to support a dumb compiler. Plus, it’s entertaining.

Another comment¶

/*
sizeof ((x) ? 1 : 0) makes the compiler still parse the expression even
without assertions enabled, so the code is always checked at compile time, but
doesn't actually generate code for it, so there are no side effects or
expensive checks at run time, just the constant size of what int would be,
which presumably gets optimized out as unused.

(The `? 1 : 0` is so this always dithers down to `sizeof (int)`...otherwise
you'll have problems when bitfields are used as the condition, since the
compiler will error out on the sizeof.)

This also solves the problem of...

    int somevalue = blah();
    SDL_assert(somevalue == 1);

...which would cause compiles to complain that somevalue is unused if we
disable assertions.
*/
// [...]
#define SDL_disabled_assert(condition) \
    do { (void) sizeof ((condition) ? 1 : 0); } while (SDL_NULL_WHILE_LOOP_CONDITION)

This is a pretty big block comment explaining why the code is the way it is. Note how it doesn’t say that this is the macro called when an assertion is disabled. That’s obvious, and the reader is a maintainer who should know that. Instead it talks about why it was implemented in this way.

It’s not API documentation, but good comments are good too.

Conclusion¶

I hit one of each type of thing, but some of the other functions in the document have other kinds of information given about them. For example, SDL_GetAssertionHandler goes into detail about the output argument, when it will be NULL and when it’s ok to pass in NULL.

After that, SDL_GetAssertionReport has an example of how it’s used and goes into detail about its return value. It even talks about the specific way in which it is not threadsafe.

There’s also the fact that this is a header file, the correct place to document a C API.

I talked a lot about what was there, but just as important is what isn’t. Nowhere is the fact that a function has no return value pointed out. The comment blocks don’t duplicate the names of the functions (or the file) or just mechanically repeat the names of their arguments without adding any information.

There aren’t boilerplate fields that must be filled out, but whose contents don’t matter. Nowhere is the author of a function named. The description of the function is just a description; it doesn’t need 3 lines to say DESCRIPTION before telling me what it does. This documentation treats the reader like a person who wants to know how to use an API, not a person with a checklist who wants to make sure all the documentation elements are present.

This is good documentation.

Previous: Bad Comments

Recent Posts

Tags