Canonical Function Structure
Here is a proposed canonical structure for functions/procedures. Clearly in some cases some sections would not be present. What’s important is to understand the basic phasing of the work to be done and the separation of tasks.
int foo(
char *StringIn, // “in” parameter
size_t BufferLength, // “in” parameter
char Buffer[], // “out” parameter, max length governed by BufferLength
size_t *CharsWritten // “out” parameter, must be <= BufferLength
) {
// Phase 1: Initialize out parameters
*CharsWritten = 0;
// Phase 2: Capture and validate parameters
const size_t StringInLength = strlen(StringIn);
if ((BufferLength != 0) && (Buffer == NULL))
return ERR_INVALID_PARAMETER;
// Phase 3: do work
if (BufferLength <= StringInLength)
return ERR_BUFFER_TOO_SMALL;
memcpy(Buffer, StringIn, StringInLength);
Buffer[StringInLength] = ‘\0’;
// Phase 4: copy data out
*CharsWritten = StringInLength + 1; // safe because BufferLength is > StringInLength so “+1” won’t overflow
// Phase 5: goodbye
return ERR_SUCCEEDED;
}
There may be other phases and the example’s perhaps kind of dumb but I think it works well to call out what kinds of operations should happen in which phases and even more so what doesn’t have to happen later on given work that occurred earlier.
Comments
Anonymous
October 07, 2006
I have never thought about it formally, and you are right, but isn't this kicking in an open door? I have read your articles on the dll loader. They are at expert level. This article is more of the 'this is a compiler. it turns source code into executable code' level. Is this perhaps the start of a new series of articles? Good to see that you are writing again.Anonymous
October 20, 2006
Ignoring that this is a trivial example, I have a couple of questions about the style... Are you intentionally barfing on bad values of CharsWritten (i.e. null) to allow for early and obvious error detection? It looks like StringInLength validation is occurring in the "do work" stage, I think of that more as parameter validation. In my ideal world, the "do work" stage doesn't do anything that can fail AND be observed by a caller of the function (as long as you are in a good state at the start of the call), i.e. some kind of transactional guarantee.Anonymous
November 01, 2006
Didn't you mean '(BufferLength == 0)' instead of '(BufferLength != 0)' in Phase 2? (Or even < 1 since you're planning to copy the terminal NULL). Wouldn't be better to put the required buffer size in CharsWritten before returning ERR_BUFFER_TOO_SMALL? That way the caller may first want to check the required buffer size, than allocate a largely enough buffer and call again and expect to succeed. Wouldn't 'memcpy(Buffer, StringIn, StringInLength+1);' have the same effect as 'memcpy(Buffer, StringIn, StringInLength); Buffer[StringInLength] = ‘�’;' ? I would also check source and destination buffers to not overlap.Anonymous
November 06, 2006
To Raul Igrisan: if ((BufferLength != 0) && (Buffer == NULL)) diagnoses an error when the caller pretends to have a nonzero length of memory at location NULL. This test allows the caller to have no memory at all at location NULL. A later test diagnoses an error if BufferLength is actually zero. > I would also check source and destination > buffers to not overlap. I agree on this one. Mr. Grier called memcpy() not memmove() so his code can yield undefined behaviour unless he adds this validation.Anonymous
November 10, 2006
Norman, Raul - that depends entirely on the documented contract for the function, which is lacking here; if that says "the input and output buffers may not overlap", then there you have it.