How do you convey intent regarding pointers?

Question

I've always used the tried-and-true "Document and Pray" approach when it comes to conveying the fact that a pointer result from a function (either as a return value or a reference parameter) should not be freed by client code.

The other day someone showed me this function prototype:

const struct foo * const bar(void);

When I pointed out that the const qualifier on the pointer was stripped and ignored by the compiler (along with generating a warning with -Wall), they said, "I know, I use it to let consumers know that they shouldn't free the pointer." And the kicker is that they really didn't seem to think there was a problem with it.

My question is whether or not it could be considered common practice (I've never seen it before) to use this sort of jiggery-pokery to try and get superfluous or even questionable language constructs to "self-document" an API?

If you were consuming an API with this prototype in it, would you come away with the notion that the pointer was "hands off", or that the library writer was in dire need of medication?

Answer 1

In general I fully support annotations, even if the compiler doesn't do anything different with them. I agree with your coworker that it's a good idea that can help clarify the code. I'm not sure that their particular annotation is helpful, unless most of your team comes from a C++ background, though. I really don't like that it generates a warning.

Unfortunately, C allows too many unsafe ways to use pointers. Even more unfortunately, sometimes those unsafe ways of using pointers are the best solution to a given problem. So your options are to write them anyway and pray, as you say, or to try to find ways to minimize them and document them when they are needed. If you can get the compiler to help you, all the better!

One way to minimize these types of issues is to use opaque pointers. For example, let's say you have a library function that returns a pointer to some memory that represents the pixels of an image. Instead of returning a raw pointer, you could return something called an ImageRef. What's an image ref? To a user of the library it's an opaque object that the header defines like so:

typedef struct image_t* ImageRef;

What's an image_t? Users of the library have no idea. It's never defined for them. Since ImageRef is a pointer, it doesn't need to be. You can write functions that take an ImageRef and other data and perform work for the caller rather than just giving them a pointer and going to town. Behind the scenes, it can be defined as:

struct image_t {
    int width;
    int height;
    int bytesPerPixel;
    unsigned char* pixels;
};

And your library can have access to that definition and be very careful about how it uses the pixels pointer internally, but publicly, if the definition of that structure isn't published anywhere, there's no way to get at the pointer and mess things up.

A couple other things I forgot to mention. You can use assertions in your debug builds to ensure things like that pointers are not NULL. Every function that takes a pointer can have an assertion as its first line:

assert (ptr != NULL);

This will help you find issues during debugging, hopefully before you release.

Another thing you can do is nullability annotations, even if they're not supported by your language. In C you can simply make a couple of macros:

#define NONNULL
#define NULLABLE

You can then put those in the function prototypes for functions that take pointers:

SomeStruct* NULLABLE foo(SomeType* NONNULL somePtr);

The above will let a developer calling the function know that somePtr must not be NULL. It's on them to check it before calling the function. It also lets them know that the return value may be NULL so they have to check it on return. You could come up with your own annotations defined the same way for things like passing ownership to the caller. Perhaps something like:

#define STRONG // Caller must free
#define WEAK   // Only a reference, caller must not free

Since these are all #defined to nothing, the compiler ignores them, but readers can see what they mean. It would be nice if the compiler could help with enforcement, but it's better than nothing.

FWIW, this Stack Overflow Question seems to indicated that MSVC, llvm, and gcc all have some sorts of annotations for C. It looks like they're all different, but perhaps some preprocessor magic could unite them for your code (or maybe you can standardize on a single compiler)?

Answer 2

Shoulds vs. Should Nots

I've always used the tried-and-true "Document and Pray" approach when it comes to conveying the fact that a pointer result from a function [..] should not be freed by client code.

In my opinion it's a little easier to focus on documenting when users should free a function's output than when they shouldn't. Focusing on "should nots" often opens you up to paranoia contemplating about endless "what if" scenarios as opposed to "shoulds". In C, there's an endless list of things that users shouldn't do with a library, so I think you'll save a lot of time and paranoia with documentation by focusing predominantly on what they should be doing.

That philosophy applies in general to flexible systems. For example, my system is an entity-component system. That has the benefit of so much flexibility but it also opens up doors to nonsensical cases. Like in mine, a scripter can even attach a motion component to a GUI control and have the physics system apply to it. That makes no sense (it would do something physics-related in the scene and wouldn't crash or anything of that sort but would do something really weird to apply physics to the UI), and so instead of trying to handle all those weird scenarios either by forcefully implementing endless system constraints or documenting every single thing that users shouldn't do, I just focus on what they should be doing instead. If they stray off the beaten path and start trying all kinds of things other than what they should do, then they're on their own because it would take a near-infinite amount of time to document all the things that users shouldn't do in a system that offers near-infinite possibilities.

Conventions, Consistency

What I do in my C APIs is always put a *_create function right next to a *_destroy function in the header, always favoring that kind of symmetrical design convention when possible. As a result I'm focusing more on the convention making it obvious when clients should destroy things than when they shouldn't. I even have rare cases where the _create function returns a structure by value instead of a pointer to it where no destruction logic is needed, but I still provide an empty _destroy function just to preserve the symmetrical design convention. By preserving that symmetry at all times, even when it is not required, the users don't have to worry about which cases involving a create function require a corresponding destroy function to be called. There's always a corresponding destroy function to be called and proper usage dictates that it is called, even when the destroy function does nothing at all, even when I strongly believe that the destroy function will never need to do anything at all.

If I broke symmetry just once in the SDK, then it would raise endless questions about which create functions which a corresponding destroy to be called -- suddenly the intellectual overhead skyrockets with just one exceptional case. And that leads me to a thought where:

A convention applied with the utmost consistency can more effectively communicate usage patterns than any amount of documentation.

However, it requires utmost consistency, since as with the above scenario, a single exception to the rule suddenly destroys all the benefits. 99% consistency is arguably even worse than 0% consistency, since the 99% scenario might have users stop asking questions except when things go wrong, at which point they might be asking endless paranoid questions. The 0% consistency case at least has them asking questions in every single case. 100% consistency is the ideal, and 99% consistency is arguably the worst thing possible.

As a blatant example of why 99% consistency is so bad, we're all used to a floppy disk icon being used to save files. Well, imagine if there was just one rare case where someone used it to format your hard drive. 99% consistency is a horrible thing. I've often been the annoying nit in team environments when it comes to demanding consistency and documented conventions just in the SDK (the public APIs used far beyond our team), but it's because I see 99% consistency as worse than 0%.

It's a fairly rare case in my system for a function that doesn't fit the above convention to output anything with the client's responsibility to call another function to free the memory (actually maybe non-existent, I'd have to double-check), and in no cases in my API should the client ever use free on anything related to the SDK. That could cause problems across module boundaries. So even for functions that output variable-sized strings, I provide a corresponding symmetrical function to destroy the string. It is never up to the client to free directly.

However, I try to minimize the number of cases that leave the client to destroy anything manually that don't fit the symmetrical *_create and *_destroy convention. Whenever possible, I prefer to receive a pointer to write to with memory already allocated by the client (on stack or heap). If the output is variable-sized, then I have a separate function typically that communicates the size of the data the client needs to allocate and free themselves prior to calling the function which fills out that array.

Self-Documenting Code

That's just what has worked best for me personally, though on to your question:

My question is whether or not it could be considered common practice (I've never seen it before) to use this sort of jiggery-pokery to try and get superfluous or even questionable language constructs to "self-document" an API?

I think short of very symmetrical conventions applied consistently everywhere, most C code can't practically hope to be self-documenting about issues like these (which relate to client responsibilities). const can be very useful to convey read-only intentions, but I don't find it very clear or useful to convey resource management responsibilities to the client. After all, even a pointer to a const struct Foo* could still have a field that needs to be freed, and such a field would then only be read-only with respect to the pointer, not the pointee. The pointer field would be free to be copied and the pointee would be free to be modified at any given point.

Just in general a pointer can represent so many different things. Is struct Foo* a pointer to a contiguous array of Foos or a pointer to a single Foo? The type never communicates the difference, and so I prefer to use struct Foo array[] or something to this sort in function parameters as opposed to struct Foo* array. It makes no difference at compile-time, but it does help a little bit in communicating the idea that the function expects an array.

C++ developers might have an easier time with self-documenting code in this respect since they have their classes with destructors and copy constructors and all that stuff, but I wouldn't want any of it in C as the appeal of C to me is being able to work with the type system in a way that can assume that none of this stuff exists, like being able to memcpy data without having to know anything about it. That inevitably comes with the trade-off that we have to establish clear conventions and documentation about client responsibilities. It's something to accept as I see it and come up with clear documentation standards rather than trying too hard to come up with self-documenting code, at least in this one area.

Answer 3

Alright, the takeaway I'm getting is pretty much my initial opinion: language constructs are best used to convey intent when they are designed to convey intent.

int foo(const char *bar);

The const qualifier in this prototype conveys -- by design -- the implicit contract that the function won't modify bar.

Absent that, documentation takes the day. Whether it's well-documented symbolic annotations, or the complete absence of information with respect to opaque pointers, or simply thorough documentation for the particular API.

Elegant is one thing; then there's just plain squirrelly.

Answer 4

I would tend to find it quite confusing to use const to indicate that a pointer should not be freed, especially if that was considered some form of self-documentation. The way I like to tackle this (and I really wish more C APIs favored this), is to uniformly avoid functions returning pointers to memory that should be freed directly by the client with free, by favoring this type of style:

// Returns a new 'foo' object.
foo* create_foo(allocator* alloc)
{
    return allocate(alloc, sizeof(Foo));
}

// Returns a string containing the absolute path given the relative path,
// or null otherwise.
const char* find_path(allocator* alloc, const char* rel_path);

The key thing to note here is the allocator parameter. All API functions in our systems that allocate memory dynamically and return a pointer to it avoid using malloc and free directly and instead go through the allocator and take a pointer to it as a parameter.

This also improves efficiency quite a bit and wipes malloc/free-related hotspots off the map as well as reducing cache misses with our access patterns, as the allocator is a simple sequential allocator that only needs to offset a pointer and return its previous address to pool memory from contiguous blocks and, furthermore, it uses a 256-byte small buffer for its initial contiguous block, allowing that first block to be allocated on the stack without even requiring a heap allocation.

It also tends to simplify error-handling a lot in functions that allocate multiple objects, since we can reuse the same allocator for all of them and just free all the memory for everything that was successfully allocated with a simple call to purge(alloc) instead of having to free individual pointers. The downside, of course, is that we have to deal with this separate allocator object, yet I find this simplifies things a tremendous deal in most of our cases especially when it comes to freeing everything properly in error paths.

Provided it is used uniformly in our API with a strict standard, I think it makes it quite explicit what functions actually allocate memory dynamically that needs to be freed by the caller without having to document this requirement in each individual one.