Function Wrapper: Opinions and Critique Desired

[Chris Torek]

static int func_recurs(int arg1, int arg2, int prev_pos)
{ ... }

The only effect of the "static" keyword here is that the function name
"func_recurs" is not visible outside the current translation unit.
(Making it more globally visible would probably be harmless, but it
clutters the namespace.) There's no effect on allocation.

Indeed.

For what it is worth, the "static" and "extern" keywords are rather
squirrelly. I have an in-progress HTML article on this but it is
not yet done and may contain errors:

http://web.torek.net/torek/c/tmp.html

Incidentally, in Lisp-y languages, one tends to write the
"helper function" entirely within the "external-interface function",
using lambda or similar to write the internal function without
even bothering with a name. This method is not available in C,
but using a "static" function, we can fake it.

 

[Michael Wojcik]

Now here you had me scratching my head (and searching the FAQ).

So if I read you correctly, your recommending that I define the
functions like this:

int func_lazymemory(int arg1, int arg2)
{ ... }

...and...

int func_recurs(static int arg1, static int arg2, int prev_pos)
{ ... }

...so that storage will only be allocated once for both arg1 and arg2.
Correct?

No, I just meant:

static int func_recurs(int arg1, int arg2, int prev_pos)
{ ... }

int func_lazymemory(int arg1, int arg2)
{ ... }

That way, the identifier func_recurs is only visible to other functions
in the same source file - in fact, to other functions below the first
declaration of it in that source file, which is why I moved it above
func_lazymemory.

The "static" will catch any places where you try to call func_recurs
from another source file. In effect, you're saying "func_lazymemory
is what you call when you want this functionality".

You can't declare a parameter with the "static" storage-class
specifier. (The only storage-class specifier you can use with a
parameter is "register", IIRC, and "register" is rarely useful in
modern C implementations.)

If so, this handily provides and alternate answer to question 1b from my
original post (which I just now noticed I bungled) where I was trying to
pass pointers to conserve storage:

You're probably not conserving any storage in this case. It would
depend on how your C implementation passes parameters and various
other things outside the scope of the C language, but it's unlikely
to be worth doing.

On the other hand, if func_recurs changes the values of arg1 and
arg2, and you want func_lazymemory to see those changes, then you
would want to pass their addresses so that func_recurs actually
updates the objects that func_lazymemory sees.

--
Michael Wojcik (e-mail address removed)

Unfortunately, as a software professional, tradition requires me to spend New
Years Eve drinking alone, playing video games and sobbing uncontrollably.
-- Peter Johnson

 

[websnarf]

A while back, I was mucking around with a recursive function. For
brevity's sake, I'll define it like this:

int func_recurs(int arg1, int arg2, int prev_pos)
{
int crnt_pos = 0;
int result;

/* stuff happens to arg1, arg2, and crnt_pos */

func_recurs(arg1, arg2, (prev_pos + crnt_pos) );

return result;
}

The critical part here (as I'm sure you've figured out already) is that
prev_pos (the previous position) of the last recursion is combined with
crnt_pos (the current positon) of the present recursion and passed along
to become the prev_pos of the next recursion.

But written this way, I had to remember that any call to func_recurs()
always had to look like this:

func_recurs(arg1, arg2, 0);

Frankly, I thought that constant 0 dangling at the end was rather
inelegant. Then it struck me that maybe I could "hide" func_recurs
inside another function so that I wouldn't have to strain my brain
remembering about that pesky third argument; something like:

func_lazymemory(int arg1, int arg2)
{
func_recurs(arg1, arg2, 0);
}

Being new to C, I felt pretty clever for coming up with this all on my
own. But then I thought: "Bah! Smells of kludge. There must be a
better way."

What? This is the way to do it. This is not a kludge, this is just
how you have to do it sometimes. Try implementing mergesort some time
-- this is just the way you do it. Nothing kludgy about it. If you
want to make it have less overhead or something do it like this:

#define func_lazymemory(arg1,arg2) func_recurs(arg1, arg2, 0)

So I changed it to:

int func_recurs(int arg1, int arg2)
{
--> static int prev_pos; /* conveniently init'd to 0 on 1st call */

Yes ok, so you can only ever call this function once. In general, you
should not declare static elements in your stack unless 1) they are
read-only, 2) you can guarantee that your function will not be called
in a multithreaded environment, 3) their relevant life-time is only
inline and in a multi-threaded environment they are guarded by some
critical section anyhow.

int crnt_pos = 0;
int result;

/* stuff happens to arg1, arg2, and crnt_pos */

--> prev_pos += crnt_pos; /* adjust for next recursion */
--> func_recurs(arg1, arg2);
--> prev_pos -= crnt_pos; /* restore for current recursion */

return result;
}

Drawing your attention to the four "-->" lines, I then thought: "Boy is
that ugly", but I no longer felt I'd somehow cheated inside my code. I
packed away the function and forgot about it ... until tonight.

Whenever you rely on statics to retain state from multiple contexts you
should always be wary. If I ever saw this, I would by default assume
it was a bug, and I would rewrite it in the wrapper style you referred
to above.

Tonight I was browsing through some months' old threads on clc and came
across one that discussed function wrappers. "Cool," I thought. A
response referenced the FAQ, so I pulled it up and had a read. So, my
clever idea is old hat. Figures! :-D

I started thinking again about how I dealt with that function and came
up with some questions and concerns:

1a) Is my use of a wrapper appropriate (or am I ignorant of some wrapper
rule or style convention)?

Yes, in fact that is the way I would recommend. (Or using the macro,
but it depends on how you want to expose the interface.)

1b) Wouldn't the wrapper be improved (and the program conserve some
storage) if it were re-written to pass pointer values like this?

func_lazymemory(int *arg1, int *arg2)
{
func_recurs(arg1, arg2, 0);
}

No. Just copy the parameters.

2a) Is one of my solutions better/cleaner/preferable over the other?

Yes, the wrapper method is far better, because you are not using
writable function-local statics.

2b) Or are they equivalent, and picking one is a matter of (my) personal
style?

They are not equivalent, because the second solution has a high failure
risk (through multithreading, or your wrong assumption about how the
static is initialized.)

3) Is there some other (maybe obvious) solution to ditching that third
argument that I've somehow overlooked?

No, just leave it.

 

[Ben Pfaff]

The critical part here (as I'm sure you've figured out already) is that
prev_pos (the previous position) of the last recursion is combined with
crnt_pos (the current positon) of the present recursion and passed along
to become the prev_pos of the next recursion.

But written this way, I had to remember that any call to func_recurs()
always had to look like this:

func_recurs(arg1, arg2, 0);

Frankly, I thought that constant 0 dangling at the end was rather
inelegant. Then it struck me that maybe I could "hide" func_recurs
inside another function so that I wouldn't have to strain my brain
remembering about that pesky third argument; something like:

func_lazymemory(int arg1, int arg2)
{
func_recurs(arg1, arg2, 0);
}

What? This is the way to do it. This is not a kludge, this is just
how you have to do it sometimes. Try implementing mergesort some time
-- this is just the way you do it. [...]

Hmm? That might be one way to do it. But there's no reason that
a merge sort even has to be recursive. For example, take a look
at the merge sort in my linked list library:

/* Sorts R0...R1 into ascending order
according to COMPARE given auxiliary data AUX.
In use, keep in mind that R0 may move during the sort, so that
afterward R0...R1 may denote a different range.
(On the other hand, R1 is fixed in place.)
Runs in O(n lg n) time in the number of nodes in the range. */
void
ll_sort (struct ll *r0, struct ll *r1, ll_compare_func *compare, void *aux)
{
struct ll *pre_r0;
size_t output_run_cnt;

if (r0 == r1 || ll_next (r0) == r1)
return;

pre_r0 = ll_prev (r0);
do
{
struct ll *a0 = ll_next (pre_r0);
for (output_run_cnt = 1; ; output_run_cnt++)
{
struct ll *a1 = ll_find_run (a0, r1, compare, aux);
struct ll *a2 = ll_find_run (a1, r1, compare, aux);
if (a1 == a2)
break;

a0 = ll_merge (a0, a1, a1, a2, compare, aux);
}
}
while (output_run_cnt > 1);
}

There's a lot that goes unsaid here, but you can find the whole library at
http://cvs.savannah.gnu.org/viewcvs/pspp/src/libpspp/ll.c?rev=1.1&root=pspp&view=log
if you're curious.