Is it better to use the returned value of a function directly and not store it in a variable?

I'm having a discussion with a colleague about how to structure the return code checks in our code base. In a lot of places the code looks like this:

logOnError(functionWithLotsOfParameters(a, b, c, d, etc),
           "Error Message", module_name, some_more_stuff);

I think this kind of buries what is actually happening here, which is the function call of the inner function. I would like to structure the code like this:

ReturnType ret;
ret = functionWithLotsOfParameters(a, b, c, d, etc);
logOnError(ret, "Error Message", module_name, some_more_stuff);

ReturnType in this context is basically just an unsigned int. My colleague argues that this increases the stack size of the function unnecessarily, which could be an issue, because this runs on an embedded system and we're somewhat memory constrained.

My counter argument is that even in the first case the return value has to be somewhere in memory, so I don't think that the footprint would increase.

Who's right?

It is general good practice to split complex expression into several, across several lines. Readable, maintainable code is almost always more important than stack usage, or even execution speed. Also, micro-optimizations like these are often not worth the effort - assume that the compiler's optimizer will take care of it unless you have good reasons to believe otherwise.

Notably, if stack usage was important, you shouldn't use such a heavy API to the function in the first place, but pass a single pointer to struct instead. This is actually a stack use optimization technique sometimes used in some very limited systems like low-end 8-bit microcontrollers. PIC for example is infamous for its dysfunctional stack implementation, where in addition to size you also had to keep track of call stack depth.

You are correct in assuming that returned values cannot be stored in thin air - every value used by the program has to be allocated somewhere, no matter if stored in a named variable or an anonymous temporary location. Not necessarily on the stack though, could as well be inside a register, depending on ABI and calling convention.

Testing your artificial code in an even more artificial context https://godbolt.org/z/bYsE1WT7e...

• gcc x86_64, first version actually somewhat slower
• clang x86_64, identical code for both versions.
• gcc ARM32, identical code for both versions.
• gcc AVR, first version actually somewhat slower

Why the first version was slower on some systems is because of ABI and how the specific compiler optimizes according to the ABI. The ABI states where the return code has to be stored and where the first parameter has to be stored: different places. That in turn could mean that the compiler has to shuffle around the data a bit between function calls.

Lesson learnt is that overthinking/over-engineering various micro-optimizations and trying to write less readable code for the sake of performance can have the opposite effect. The general best practices are:

• Write code as simple and readable as possible. Readable, simple code often has the best performance.
• Don't optimize manually unless you have actually found a performance problem.
• Don't optimize manually unless you actually have somewhat in-depth knowledge of the target CPU and ABI.

For example I was about to make a cocksure statement that these snippets would surely be optimized identically, but then it turned out they weren't on some targets.

In a symbolic debugger (which you really should be using), it can be cumbersome to inspect or infer the return value if not assigned. Also "step-into" operations work at statement level, so in your example step-into would enter functionWithLotsOfParameters() first, which can be cumbersome if you only intended to step-into logOnError().

Style, practice and readability arguments are a matter of opinion, while performance arguments are questionable; the debugger experience is a matter of fact. Habitually coding in a style that supports debug is clearly a good idea.

Moreover the argument holds regardless of the number of parameters the function has, which is an irrelevance (although it's advisability is a separate issue). The presentation of code with long lists of any kind is better dealt with with clear whitespace layout (i.e. newlines, indentation), and is even more so a matter of opinion.

It depends on the optimisation level you use.

Let's try and see what the compiler gives use. For this test I used the following code :

#include <stdio.h>
#include <stdlib.h>

int __attribute__ ((noinline)) square(int num) {
    return num * num;
}

void __attribute__ ((noinline)) log_val(int a, int b, int c, int d)
{
    printf("LOG: %i %i %i %i", a, b, c, d);
}

void test_0(int val)
{
    int retval;
    retval = square(val);
    log_val(0,1,2,retval);
}

void test_1(int val)
{
    log_val(0,1,2,square(val));
}

Using GCC for ARM (as I am more familiar with ARM assembly) with flag -O0, we get :

test_0:
        push    {r7, lr}
        sub     sp, sp, #16
        add     r7, sp, #0
        str     r0, [r7, #4]
        ldr     r0, [r7, #4]
        bl      square
        str     r0, [r7, #12]
        ldr     r3, [r7, #12]
        movs    r2, #2
        movs    r1, #1
        movs    r0, #0
        bl      log_val
        nop
        adds    r7, r7, #16
        mov     sp, r7
        pop     {r7, pc}
test_1:
        push    {r7, lr}
        sub     sp, sp, #8
        add     r7, sp, #0
        str     r0, [r7, #4]
        ldr     r0, [r7, #4]
        bl      square
        mov     r3, r0
        movs    r2, #2
        movs    r1, #1
        movs    r0, #0
        bl      log_val
        nop
        adds    r7, r7, #8
        mov     sp, r7
        pop     {r7, pc}

We can see that the first version increases the stack by 16 bytes (sub sp, sp, #16), and the second by 8 bytes (sub sp, sp, #8). The stack is, in fact, bigger with a separate variable.

But this is with no optimisation at all. Now let's see what happens when we enable the lowest optimisation level, -O1 :

test_0:
        push    {r3, lr}
        bl      square
        mov     r3, r0
        movs    r2, #2
        movs    r1, #1
        movs    r0, #0
        bl      log_val
        pop     {r3, pc}
test_1:
        push    {r3, lr}
        bl      square
        mov     r3, r0
        movs    r2, #2
        movs    r1, #1
        movs    r0, #0
        bl      log_val
        pop     {r3, pc}

Here we see that both function are absolutely identical, and don't even use the stack. Meaning that with -O1 the resulting assembly is independent of the coding style.

So your colleague is right if you don't use optimisation, but even with the lowest optimisation enabled the results are identical.

Use whichever method is the most readable, easiest to maintain, etc... But the stack size is a non-issue.